feat(router): SidebarPanel now has routerles version #4218
Conversation
WalkthroughAdds a router-disabled sidebar mode driven by internal navigation state, updates sidebar-related types (moving VERSIONS enum, changing filteredTemplateIds and optional sidebar), extends InternalSidebarNavigation, adds router-disabled SidebarPanels variant, and updates tests and mocks to support these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant SidebarPanelsWrapper as SidebarPanels
participant RouterMode as SidebarPanelsRouter
participant DisabledMode as SidebarPanelsRouterDisabled
participant Sidebar as SidebarComponent
App->>SidebarPanelsWrapper: render(props: routerDisabled?, internalSidebarNavigation?, ...)
alt routerDisabled = false
SidebarPanelsWrapper->>RouterMode: render (uses React Router path)
RouterMode->>Sidebar: choose & render based on URL path
else routerDisabled = true
SidebarPanelsWrapper->>DisabledMode: render (uses internalSidebarNavigation)
DisabledMode->>Sidebar: choose & render based on internalSidebarNavigation state
end
Note over DisabledMode,Sidebar: internal handler updates drive navigation when router is disabled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/elements/common/types/SidebarNavigation.js.flow(2 hunks)src/elements/common/types/SidebarNavigation.ts(3 hunks)src/elements/content-sidebar/SidebarPanels.js(5 hunks)src/elements/content-sidebar/__mocks__/SidebarUtils.js(2 hunks)src/elements/content-sidebar/__tests__/SidebarPanels.test.js(18 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 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.
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.
📚 Learning: `versionsidebarview` intentionally uses the `versionid` field to stay consistent with current url pa...
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.
Applied to files:
src/elements/content-sidebar/__mocks__/SidebarUtils.jssrc/elements/common/types/SidebarNavigation.js.flowsrc/elements/common/types/SidebarNavigation.tssrc/elements/content-sidebar/__tests__/SidebarPanels.test.jssrc/elements/content-sidebar/SidebarPanels.js
📚 Learning: in the box-ui-elements codebase, flow components use .flow.js type definition files, not typescript ...
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-sidebar/__mocks__/SidebarUtils.jssrc/elements/common/types/SidebarNavigation.js.flowsrc/elements/common/types/SidebarNavigation.tssrc/elements/content-sidebar/__tests__/SidebarPanels.test.jssrc/elements/content-sidebar/SidebarPanels.js
📚 Learning: rendering functions (functions that return jsx) are considered anti-patterns in react development be...
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.
Applied to files:
src/elements/content-sidebar/__mocks__/SidebarUtils.jssrc/elements/content-sidebar/__tests__/SidebarPanels.test.jssrc/elements/content-sidebar/SidebarPanels.js
📚 Learning: files with `@flow` or `@flow strict` comments at the top use flow type syntax, not typescript. flow ...
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/common/types/SidebarNavigation.js.flowsrc/elements/content-sidebar/SidebarPanels.js
📚 Learning: the box-ui-elements project uses flow for type annotations in javascript files, as indicated by @flo...
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#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.
Applied to files:
src/elements/common/types/SidebarNavigation.js.flowsrc/elements/content-sidebar/SidebarPanels.js
📚 Learning: in the box-ui-elements codebase, flow files support `import type` syntax for type-only imports. the ...
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#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).
Applied to files:
src/elements/common/types/SidebarNavigation.js.flowsrc/elements/content-sidebar/SidebarPanels.js
🧬 Code Graph Analysis (1)
src/elements/content-sidebar/__mocks__/SidebarUtils.js (1)
src/elements/content-sidebar/SidebarPanels.js (7)
filteredTemplateIds(814-814)activeFeedEntryType(332-334)activeFeedEntryType(590-590)activeFeedEntryType(770-772)activeFeedEntryId(767-767)versionId(589-589)versionId(854-854)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarPanels.js
[error] 46-49: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 130-130: type arguments are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 484-484: type arguments are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 485-485: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 487-487: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 489-489: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 491-491: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 493-493: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 495-495: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 497-497: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 499-499: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 515-515: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 515-515: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 530-530: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 539-539: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 539-539: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 548-548: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 548-548: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 552-552: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 582-582: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 659-659: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 659-659: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 877-877: type arguments 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 (24)
src/elements/content-sidebar/__mocks__/SidebarUtils.js (3)
25-34: LGTM! Good test utility enhancement.The addition of data attributes to the mock component improves testability by exposing the
filteredTemplateIdsprop. The conditional rendering and JSON stringification are properly implemented.
41-52: LGTM! Comprehensive data attribute exposure.Good addition of multiple data attributes for testing the ActivitySidebar mock. All relevant props are properly exposed.
54-63: LGTM! Consistent implementation.The VersionsSidebar mock properly exposes the
versionIdprop as a data attribute, maintaining consistency with other mock components.src/elements/common/types/SidebarNavigation.js.flow (3)
16-21: LGTM! Logical reorganization of the VERSIONS constant.Moving
VERSIONSfromViewTypetoFeedEntryTypemakes semantic sense as versions are now treated as a feed entry type rather than a view type. This aligns with the new navigation structure.
26-33: LGTM! Improved type flexibility.The changes improve type definitions:
filteredTemplateIdsas an array allows multiple template filtering- Optional
sidebarproperty supports scenarios where no sidebar is active
35-38: LGTM! Support for silent navigation.The optional
silentproperty enables controlled navigation state changes without triggering side effects, which is useful for the router-disabled mode.src/elements/common/types/SidebarNavigation.ts (4)
11-16: LGTM! Consistent with Flow types.The addition of
VERSIONStoFeedEntryTypemaintains consistency with the Flow type definitions and supports the new navigation approach.
18-22: LGTM! Proper type constraints for version views.The changes correctly enforce that version views must specify
activeFeedEntryType: FeedEntryType.VERSIONSand makeversionIdoptional, which aligns with the navigation requirements.
24-27: LGTM! Stricter typing for metadata views.Making
filteredTemplateIdsa required array property provides better type safety and clearer API contracts.
41-53: LGTM! Enhanced navigation type flexibility.The optional
sidebarproperty and newsilentflag properly support both router-based and router-disabled navigation modes.src/elements/content-sidebar/__tests__/SidebarPanels.test.js (6)
1-19: LGTM! Clean test setup and imports.Good migration to React Testing Library and proper organization of imports and test setup.
39-40: Minor: Consider consistent naming for render helpers.Both
renderSidebarPanelsandrenderSidebarPanelsRouterDisabledfollow good patterns for test helpers.
514-549: LGTM! Well-structured refresh test.Good implementation using React refs and proper mocking of refresh functions for each sidebar. The test correctly verifies that the activity sidebar receives the cache parameter while others don't.
623-648: LGTM! Well-structured router-disabled mode tests.Good setup for the router-disabled mode tests with appropriate helper functions and consistent structure mirroring the router-based tests.
1184-1221: LGTM! Comprehensive refresh test for router-disabled mode.The refresh test properly validates the behavior in router-disabled mode, maintaining consistency with the router-based implementation.
1223-1329: LGTM! Thorough version change handling tests.Excellent coverage of version reset scenarios in router-disabled mode, properly testing when versions should and shouldn't be reset based on navigation changes.
src/elements/content-sidebar/SidebarPanels.js (8)
1-1: LGTM! Appropriate ESLint rule disable.The
max-classes-per-filedisable is justified since the file now contains three classes to support both router-based and router-disabled modes.
46-50: Props properly extended for router-disabled mode.The new type imports and optional props are correctly defined using valid Flow syntax to support the internal navigation state-driven approach.
Also applies to: 73-74, 82-82
130-130: Class appropriately renamed to distinguish router-based implementation.The rename from
SidebarPanelstoSidebarPanelsRouterclearly indicates this is the router-based implementation.
484-510: Consistent class structure for router-disabled implementation.The
SidebarPanelsRouterDisabledclass properly mirrors the structure ofSidebarPanelsRouterwith appropriate refs, state, and cache initialization.
515-537: Version navigation logic correctly adapted for internal state.The
componentDidUpdateandisVersionsNavigationPathmethods properly handle version state resets based on internal navigation instead of URL paths.
687-874: Render method correctly implements router-disabled sidebar selection.The implementation properly:
- Maps internal navigation state to sidebar components
- Handles panel rendering notifications
- Passes appropriate props including
routerDisabledflag- Maintains compatibility with existing sidebar components
877-887: Clean wrapper implementation for conditional rendering.The
SidebarPanelswrapper component properly delegates to the appropriate implementation based on therouterDisabledprop.
889-890: Exports properly expose both implementations.Named exports allow for targeted testing of both router-based and router-disabled components.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/elements/content-sidebar/__tests__/SidebarPanels.test.js (3)
459-459: Fix type inconsistency with filteredTemplateIdsThe
filteredTemplateIdsshould be an array of strings according to the type definitions, not numbers.- expect(metadataSidebar).toHaveAttribute('data-filtered-template-ids', JSON.stringify(['123', '124'])); + expect(metadataSidebar).toHaveAttribute('data-filtered-template-ids', JSON.stringify(['123', '124']));Note: This appears to already be correctly using strings in the assertion.
962-962: Fix type inconsistency with filteredTemplateIdsThe
filteredTemplateIdsshould be strings, not numbers, to match the type definitions.- ${{ sidebar: 'metadata', filteredTemplateIds: ['1', '3'] }} | ${true} | ${true} | ${true} | ${false} | ${true} | ${true} | ${true} | ${false} | ${'boxai'} + ${{ sidebar: 'metadata', filteredTemplateIds: ['1', '3'] }} | ${true} | ${true} | ${true} | ${false} | ${true} | ${true} | ${true} | ${false} | ${'boxai'}Note: This is already correctly using strings.
1103-1113: Fix type inconsistency with filteredTemplateIdsThe
filteredTemplateIdsshould be strings, not numbers, to match the type definitions.test('should render with filteredTemplates deeplink', () => { renderSidebarPanelsRouterDisabled({ internalSidebarNavigation: { sidebar: 'metadata', - filteredTemplateIds: [123, 124], + filteredTemplateIds: ['123', '124'], }, features: { metadata: { redesign: { enabled: true } } }, }); const metadataSidebar = screen.getByTestId('metadata-sidebar-redesigned'); - expect(metadataSidebar).toHaveAttribute('data-filtered-template-ids', JSON.stringify([123, 124])); + expect(metadataSidebar).toHaveAttribute('data-filtered-template-ids', JSON.stringify(['123', '124'])); });Note: This is already correctly using strings in both the input and assertion.
src/elements/content-sidebar/SidebarPanels.js (1)
630-630: Fix missing semicolon.Based on the past review comments, there's still a missing semicolon on line 630:
- return SIDEBAR_VIEW_VERSIONS + return SIDEBAR_VIEW_VERSIONS;
🧹 Nitpick comments (1)
src/elements/content-sidebar/SidebarPanels.js (1)
484-553: Significant code duplication in SidebarPanelsRouterDisabled constructor and methods.The new class duplicates most of the state, refs, lifecycle methods, and helper functions from
SidebarPanelsRouter. While this provides clear separation of concerns, it creates maintenance overhead.Consider extracting shared logic into a base class or composition pattern to reduce duplication:
// Example approach - extract shared functionality class SidebarPanelsBase extends React.Component { // Common state, refs, and helper methods constructor(props) { super(props); this.boxAISidebar = React.createRef(); // ... other shared initialization } // Shared methods like refresh(), setBoxAiSidebarCacheValue() }This would reduce duplication while maintaining the clear separation between router and non-router implementations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/elements/content-sidebar/SidebarPanels.js(6 hunks)src/elements/content-sidebar/__tests__/SidebarPanels.test.js(18 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 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.
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.
📚 Learning: `versionsidebarview` intentionally uses the `versionid` field to stay consistent with current url pa...
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.
Applied to files:
src/elements/content-sidebar/__tests__/SidebarPanels.test.jssrc/elements/content-sidebar/SidebarPanels.js
📚 Learning: in the box-ui-elements codebase, flow components use .flow.js type definition files, not typescript ...
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-sidebar/__tests__/SidebarPanels.test.jssrc/elements/content-sidebar/SidebarPanels.js
📚 Learning: rendering functions (functions that return jsx) are considered anti-patterns in react development be...
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.
Applied to files:
src/elements/content-sidebar/__tests__/SidebarPanels.test.jssrc/elements/content-sidebar/SidebarPanels.js
📚 Learning: in the box-ui-elements codebase, when using early returns in conditional chains (where each if block...
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4218
File: src/elements/content-sidebar/SidebarPanels.js:630-630
Timestamp: 2025-08-07T14:10:14.152Z
Learning: In the box-ui-elements codebase, when using early returns in conditional chains (where each if block contains a return statement), else keywords are considered unnecessary and should be omitted for cleaner, more readable code.
Applied to files:
src/elements/content-sidebar/SidebarPanels.js
📚 Learning: files with `@flow` or `@flow strict` comments at the top use flow type syntax, not typescript. flow ...
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-sidebar/SidebarPanels.js
📚 Learning: the box-ui-elements project uses flow for type annotations in javascript files, as indicated by @flo...
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#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.
Applied to files:
src/elements/content-sidebar/SidebarPanels.js
📚 Learning: in the box-ui-elements codebase, flow files support `import type` syntax for type-only imports. the ...
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#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).
Applied to files:
src/elements/content-sidebar/SidebarPanels.js
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarPanels.js
[error] 46-49: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 130-130: type arguments are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 484-484: type arguments are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 485-485: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 487-487: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 489-489: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 491-491: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 493-493: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 495-495: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 497-497: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 499-499: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 515-515: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 515-515: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 530-530: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 530-530: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 530-530: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 539-539: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 539-539: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 548-548: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 548-548: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 552-552: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 582-582: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 659-659: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 659-659: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 880-880: type arguments 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 (22)
src/elements/content-sidebar/__tests__/SidebarPanels.test.js (12)
3-3: LGTM: Migration to React Testing LibraryGood modernization moving from Enzyme to React Testing Library. This improves test maintainability and aligns with current best practices.
6-9: LGTM: Clear component namingThe renamed imports clearly distinguish between router-enabled (
SidebarPanelsRouter) and router-disabled (SidebarPanelsRouterDisabled) variants, making the test intent clear.
15-19: LGTM: Proper test setupThe
beforeEachhook ensures clean test state by clearing all mocks, which is essential for reliable test execution.
39-39: LGTM: Clean test helper functionThe
renderSidebarPanelshelper function provides a clean abstraction for rendering the router-enabled component with RTL.
44-66: LGTM: Comprehensive path-to-sidebar mapping testsThe parameterized tests thoroughly cover all path scenarios and their expected sidebar renderings. The test structure is clear and maintainable.
418-422: LGTM: Data attribute assertionsThe tests correctly assert on
data-*attributes exposed by the mock components, which is appropriate for testing component integration without testing implementation details.
514-549: LGTM: Robust refresh method testingThe refresh tests properly use React refs and mock functions to verify that refresh calls are propagated to all sidebar components with the correct arguments. This is a well-structured approach for testing imperative API methods.
551-621: LGTM: Component update lifecycle testingThe
componentDidUpdatetests comprehensively cover version selection behavior during navigation changes. The test cases cover both scenarios where version should be preserved and where it should be reset.
623-648: LGTM: Router-disabled test structureThe new router-disabled test section is well-organized with proper setup functions and clear separation from router-enabled tests. The mock handler setup is appropriate.
650-676: LGTM: Internal navigation state testingThe parameterized tests thoroughly cover how internal sidebar navigation state maps to rendered sidebars. This validates the new router-disabled functionality comprehensively.
1184-1221: LGTM: Consistent refresh testing for router-disabled modeThe refresh tests for router-disabled mode mirror the router-enabled tests appropriately, ensuring consistent behavior across both variants.
1223-1329: LGTM: Component update testing for internal navigationThe component update tests for router-disabled mode properly validate version selection behavior based on internal navigation state changes rather than URL paths. This is the correct adaptation for the new navigation approach.
src/elements/content-sidebar/SidebarPanels.js (10)
1-1: ESLint disable added for multiple classes per file.This is appropriate given that the file now contains three related classes that work together to provide router-enabled and router-disabled functionality.
46-50: LGTM: New import types for internal navigation.The import of
InternalSidebarNavigationandInternalSidebarNavigationHandlertypes, along with the updated import ofFeedEntryTypeandViewType, correctly supports the new router-disabled functionality.
73-74: New props added for router-disabled mode.The addition of
internalSidebarNavigation,internalSidebarNavigationHandler, androuterDisabledprops properly extends the component API to support the new navigation mode.Also applies to: 82-82
105-105: URL mapping extended with versions entry.Adding
versions: undefinedtoURL_TO_FEED_ITEM_TYPEaligns the mapping with the new internal navigation feed entry types. This ensures consistency between router-based and router-disabled modes.
130-130: Class renamed from SidebarPanels to SidebarPanelsRouter.This refactoring properly separates the router-based implementation while maintaining backward compatibility through the export alias.
530-537: Version navigation path detection logic.The
isVersionsNavigationPathmethod correctly identifies when the navigation is in versions mode by checking both the sidebar type and feed entry type. The logic properly handles the navigation structure.
687-878: Router-disabled rendering implementation.The render method in
SidebarPanelsRouterDisabledcorrectly implements conditional rendering based on internal navigation state. The approach of using individual if statements for each sidebar type provides clear separation and matches the router-based approach. The passing ofrouterDisabled={true}and navigation props to child components is appropriate.
880-890: Clean wrapper component implementation.The wrapper
SidebarPanelscomponent provides a clean interface for toggling between router-based and router-disabled modes based on therouterDisabledprop. The implementation is straightforward and maintainable.
892-893: Export structure maintains backward compatibility.The export statements properly maintain backward compatibility by exporting
SidebarPanelsRouterasSidebarPanelsComponentwhile also providing access to the newSidebarPanelsRouterDisabledcomponent. The default export continues to use the wrapper component with HOCs.
552-653: Sidebar determination logic verifiedI reviewed the
InternalSidebarNavigationvariants and confirmed:
- ActivityAnnotationsSidebarView defines
fileVersionId(used correctly in the ANNOTATIONS branch).- VersionSidebarView defines
versionId(used to gate the DETAILS vs. VERSIONS branches).- No mismatches between property names and their usage in
findSidebarToRender.- All conditional branches align with the type definitions and appear sound.
No changes required.
| @@ -93,7 +102,7 @@ const MARK_NAME_JS_LOADING_METADATA_REDESIGNED = `${ORIGIN_METADATA_SIDEBAR_REDE | |||
| const MARK_NAME_JS_LOADING_DOCGEN = `${ORIGIN_DOCGEN_SIDEBAR}${BASE_EVENT_NAME}`; | |||
| const MARK_NAME_JS_LOADING_VERSIONS = `${ORIGIN_VERSIONS_SIDEBAR}${BASE_EVENT_NAME}`; | |||
|
|
|||
| const URL_TO_FEED_ITEM_TYPE = { annotations: 'annotation', comments: 'comment', tasks: 'task' }; | |||
| const URL_TO_FEED_ITEM_TYPE = { annotations: 'annotation', comments: 'comment', tasks: 'task', versions: undefined }; | |||
There was a problem hiding this comment.
This is only there to comply with type restrictions, the value will be never used, and if it is for some reason, then it falls back with undefined. Added comment to make it clearer.
| data-filtered-template-ids={filteredTemplateIds ? JSON.stringify(filteredTemplateIds) : undefined} | ||
| /> |
There was a problem hiding this comment.
I think we shouldn't pass the ids here.
Is there is another way to test that the component was rendered with certain props?
There was a problem hiding this comment.
Any particular reason not to? It seems to be the simplest way. I can potentially create some loop to render all ids in some kind of list in DOM tree - but it feels like unnecessary complication that will achieve the same goal.
| <div | ||
| data-testid="activity-sidebar" | ||
| data-active-feed-entry-type={activeFeedEntryType} | ||
| data-active-feed-entry-id={activeFeedEntryId} | ||
| data-has-sidebar-initialized={hasSidebarInitialized} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Same here, is there is another way to test it?
There was a problem hiding this comment.
I could render it in dom as text, but on the other hand those values are never rendered directly so it seems a bit pointless.
| const wasOnVersionsPath = this.isVersionsNavigationPath(prevInternalSidebarNavigation); | ||
| const isOnVersionsPath = this.isVersionsNavigationPath(internalSidebarNavigation); |
There was a problem hiding this comment.
Can we name these constants differently so it's clear what they mean? wasOnVersionsPath and isOnVersionsPath
There was a problem hiding this comment.
It does feel fairly self explanatory:
isVersionsNavigationPath checks if current navigation is on "VersionsPath", so if it should render "Versions" panel
isOnVersionPath - flag indicating that component is now on VersionsPath
wasOnVersionPath - flag indicating, that previous state was on VersionsPath
Updated naming slightly, but not sure if it helps a lot. Let me know if you have some better ideas for naming.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/elements/content-sidebar/SidebarPanels.js (1)
634-651: Default panel precedence parity with router; early-return style is fineThe precedence order matches the router-based redirect logic. Early returns read cleanly, and the missing semicolon issue reported earlier has been resolved.
🧹 Nitpick comments (3)
src/elements/content-sidebar/SidebarPanels.js (3)
774-776: Avoid passing through unrecognized feed entry types to ActivitySidebarFalling back to the rawActiveFeedEntryType can leak unexpected values into ActivitySidebar. Align with the router version by not passing any value if the key is not in the map.
Proposed tweak:
- const activeFeedEntryType = rawActiveFeedEntryType - ? URL_TO_FEED_ITEM_TYPE[rawActiveFeedEntryType] || rawActiveFeedEntryType - : undefined; + const activeFeedEntryType = rawActiveFeedEntryType + ? URL_TO_FEED_ITEM_TYPE[rawActiveFeedEntryType] + : undefined;
131-146: Reduce duplication between Router and RouterDisabled variantsBoth classes duplicate refs, cache wiring, refresh(), handlePanelRender(), and BoxAI cache shape. Consider extracting the shared bits into a small utility or a base component to DRY things up and keep behavior in lockstep.
If helpful, I can sketch a minimal shared helper that encapsulates:
- refs creation,
- boxAiSidebarCache + setBoxAiSidebarCacheValue,
- refresh(shouldRefreshCache),
- handlePanelRender().
Also applies to: 485-500
1-1: Suppress Flow syntax errors in Biome for SidebarPanels.jsThis file is a Flow‐typed JS module (it uses the
@flowpragma,import type, and generics), but our root biome.json currently doesn’t enable Flow parsing. Biome is therefore surfacing false “TypeScript‐only syntax” errors. To silence these you can choose one of the following:• Enable Flow support globally in biome.json:
{ "$schema": "https://biomejs.dev/schemas/2.1.2/schema.json", "formatter": { "enabled": false }, + "language": { + "javascript": { "jsx": true, "flow": true } + } }• Or ignore this file with a file-level directive. At the top of src/elements/content-sidebar/SidebarPanels.js, add:
+// biome-ignore-file: suppress Flow type parse errors /* eslint-disable max-classes-per-file */ // @flowEither approach will prevent Biome from mis-parsing valid Flow syntax.
📜 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 (1)
src/elements/content-sidebar/SidebarPanels.js(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-11T16:30:10.431Z
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.
Applied to files:
src/elements/content-sidebar/SidebarPanels.js
📚 Learning: 2025-08-07T14:10:14.204Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4218
File: src/elements/content-sidebar/SidebarPanels.js:630-630
Timestamp: 2025-08-07T14:10:14.204Z
Learning: In the box-ui-elements codebase, when using early returns in conditional chains (where each if block contains a return statement), else keywords are considered unnecessary and should be omitted for cleaner, more readable code.
Applied to files:
src/elements/content-sidebar/SidebarPanels.js
📚 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-sidebar/SidebarPanels.js
🧬 Code Graph Analysis (1)
src/elements/content-sidebar/SidebarPanels.js (2)
src/elements/common/types/SidebarNavigation.ts (2)
InternalSidebarNavigation(50-53)InternalSidebarNavigationHandler(57-57)src/elements/content-sidebar/types/BoxAISidebarTypes.ts (1)
BoxAISidebarCache(4-10)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarPanels.js
[error] 46-49: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 131-131: type arguments are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 485-485: type arguments are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 486-486: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 488-488: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 490-490: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 492-492: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 494-494: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 496-496: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 498-498: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 500-500: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 516-516: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 516-516: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 531-531: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 531-531: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 531-531: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 540-540: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 540-540: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 549-549: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 549-549: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 553-553: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 583-583: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 660-660: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 660-660: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 881-881: type arguments 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 (7)
src/elements/content-sidebar/SidebarPanels.js (7)
105-107: URL_TO_FEED_ITEM_TYPE mapping with versions: undefined is acceptableThe inline comment clarifies intent. Mapping “versions” to undefined ensures alignment with the internal navigation typing without leaking an invalid type to ActivitySidebar. LGTM.
516-529: Router-disabled: correct version reset on leaving versions pathThe guard mirrors the router-based behavior and avoids stale selection by calling onVersionChange(null) when transitioning off the “versions” path. LGTM.
531-539: isVersionsNavigationPath logic mirrors router semanticsRestricting “versions” display to activity/details sidebars and the FeedEntryType.VERSIONS discriminator is accurate and keeps behavior aligned with URL-based routing. LGTM.
593-609: Activity branch gate looks correctThe gate allows:
- no deep link (undefined),
- comments/tasks deep links,
- annotations only when fileVersionId is provided.
This matches the router route constraints and prevents invalid annotation configurations. LGTM.
611-617: Details branch guard prevents accidental versions renderingExplicitly excluding FeedEntryType.VERSIONS and versionId presence before rendering the details panel is consistent with the router behavior. LGTM.
627-632: Versions rendering condition matches router routesRequiring hasVersions and (activity|details) + versions discriminator ensures parity with /:sidebar(activity|details)/versions/:versionId? routing. LGTM.
50-50: FeedEntryType and ViewType are runtime enums – no changes neededI’ve verified in src/elements/common/types/SidebarNavigation.ts that both
ViewTypeandFeedEntryTypeare declared asexport enumand thus emitted as runtime objects. The existing comparisons in SidebarPanels.js are safe.
SidebarPanel now can be used without React Router.
All tests converted to RTL.
New logic covered by Unit Tests.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores