feat(router): optional router in ActivitySidebar #4199
Conversation
|
""" WalkthroughThis change introduces an internal sidebar navigation mechanism to the ActivitySidebar component, providing an alternative to React Router-based navigation. It updates type definitions to make certain navigation properties optional, adds new props and conditional navigation logic, and refactors tests to support and verify the new navigation behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ActivitySidebar
participant InternalNavHandler
participant RouterHistory
User->>ActivitySidebar: Select annotation or update replies
alt routerDisabled and internalSidebarNavigationHandler present
ActivitySidebar->>InternalNavHandler: Call handler with navigation object
else router enabled
ActivitySidebar->>RouterHistory: Push or replace path
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes 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)
🧰 Additional context used🪛 Biome (2.1.2)src/elements/content-sidebar/ActivitySidebar.js[error] 80-80: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) [error] 1130-1130: Expected an expression but instead found '}'. Expected an expression here. (parse) [error] 1136-1136: expected Remove ; (parse) [error] 1137-1137: Unexpected token. Did you mean (parse) [error] 1138-1138: expected Remove const (parse) [error] 1140-1140: Unexpected token. Did you mean (parse) [error] 1145-1145: expected Remove if (parse) [error] 1151-1151: expected Remove ; (parse) [error] 1152-1152: Unexpected token. Did you mean (parse) [error] 1153-1153: expected Remove ; (parse) [error] 1154-1154: Unexpected token. Did you mean (parse) [error] 1196-1196: expected Remove const (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 (5)
✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-sidebar/ActivitySidebar.js (1)
1134-1143: Consider extracting file version selection logic for better readability.The implementation correctly handles both navigation modes, but the file version selection logic could be extracted into a helper method for improved readability and testability.
- let selectedFileVersionId = currentFileVersionId; - if (routerDisabled && internalSidebarNavigation) { - // Use fileVersionId from internal navigation when router is disabled - selectedFileVersionId = getProp(internalSidebarNavigation, 'fileVersionId', currentFileVersionId); - } else { - // Use router-based approach when router is enabled - const match = getAnnotationsMatchPath(location); - selectedFileVersionId = getProp(match, 'params.fileVersionId', currentFileVersionId); - } + const selectedFileVersionId = this.getSelectedFileVersionId( + currentFileVersionId, + routerDisabled, + internalSidebarNavigation, + location, + getAnnotationsMatchPath + );Add this helper method to the class:
getSelectedFileVersionId = ( currentFileVersionId: string, routerDisabled: ?boolean, internalSidebarNavigation: ?InternalSidebarNavigation, location: Object, getAnnotationsMatchPath: Function ): string => { if (routerDisabled && internalSidebarNavigation) { // Use fileVersionId from internal navigation when router is disabled return getProp(internalSidebarNavigation, 'fileVersionId', currentFileVersionId); } // Use router-based approach when router is enabled const match = getAnnotationsMatchPath(location); return getProp(match, 'params.fileVersionId', currentFileVersionId); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/elements/common/types/SidebarNavigation.ts(1 hunks)src/elements/content-sidebar/ActivitySidebar.js(9 hunks)src/elements/content-sidebar/__tests__/ActivitySidebar.rtl.test.js(1 hunks)src/elements/content-sidebar/__tests__/ActivitySidebar.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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#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/common/types/SidebarNavigation.ts (2)
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-sidebar/ActivitySidebar.js (3)
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.
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.
🪛 Biome (2.1.2)
src/elements/content-sidebar/ActivitySidebar.js
[error] 80-80: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 1130-1130: Expected an expression but instead found '}'.
Expected an expression here.
(parse)
[error] 1137-1137: expected } but instead found ;
Remove ;
(parse)
[error] 1138-1138: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 1140-1140: expected } but instead found const
Remove const
(parse)
[error] 1142-1142: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 1147-1147: expected } but instead found if
Remove if
(parse)
[error] 1148-1148: expected } but instead found const
Remove const
(parse)
[error] 1149-1149: expected } but instead found :
Remove :
(parse)
[error] 1150-1150: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 1152-1152: expected } but instead found ;
Remove ;
(parse)
[error] 1155-1155: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 1157-1157: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 1158-1158: expected } but instead found ;
Remove ;
(parse)
[error] 1159-1159: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 1139-1139: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
[error] 1201-1201: expected } but instead found const
Remove const
(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 (6)
src/elements/common/types/SidebarNavigation.ts (1)
21-26: LGTM! MakingactiveFeedEntryIdoptional aligns with the internal navigation feature.The change to make
activeFeedEntryIdoptional is appropriate for supporting navigation scenarios where the active feed entry may not be specified, particularly when using the internal sidebar navigation mechanism instead of React Router.src/elements/content-sidebar/__tests__/ActivitySidebar.test.js (1)
6-7: RTL tests coverupdateRepliesandhandleAnnotationSelect
Confirmed thatActivitySidebar.rtl.test.jsincludes:
- A
describe('updateReplies()')block testing theupdateFeedItemAPI and router-disabled behavior- A
describe('handleAnnotationSelect()')block covering both default and router-disabled scenarios- An import and use of
formattedRepliesfrom../fixturesNo further action required.
src/elements/content-sidebar/__tests__/ActivitySidebar.rtl.test.js (1)
1-522: Excellent RTL test implementation with comprehensive coverage!The new RTL test suite provides thorough coverage for both
handleAnnotationSelectandupdateRepliesmethods, properly testing:
- Router-enabled and router-disabled scenarios
- Edge cases (null file versions, same file versions, missing annotations)
- Navigation state transitions
- API interactions and callbacks
The test structure is clean, well-organized, and follows RTL best practices.
src/elements/content-sidebar/ActivitySidebar.js (3)
554-590: Well-implemented conditional navigation in updateReplies!The method correctly handles both router-based and internal navigation scenarios when hiding replies. The logic properly detects when the active feed entry needs to be cleared and uses the appropriate navigation mechanism.
1147-1160: Excellent navigation handling with proper type safety!The conditional navigation logic correctly uses the enum types and constructs a well-formed navigation object. The fallback to router-based navigation is properly implemented.
1200-1217: Good prop propagation to AddTaskButton component.The navigation-related props are correctly passed down to maintain consistent navigation behavior throughout the component tree.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/elements/content-sidebar/ActivitySidebar.js (1)
134-156: Fix critical syntax errors in handleAnnotationSelect method.Static analysis shows multiple parsing errors in this code segment that would prevent the code from compiling/running. The conditional logic structure appears malformed.
The current code has syntax issues. Please verify the conditional logic structure and ensure proper JavaScript syntax. The intended logic should be:
- let selectedFileVersionId = currentFileVersionId; - if (routerDisabled && internalSidebarNavigation) { - // Use fileVersionId from internal navigation when router is disabled - selectedFileVersionId = getProp(internalSidebarNavigation, 'fileVersionId', currentFileVersionId); - } else { - // Use router-based approach when router is enabled - const match = getAnnotationsMatchPath(location); - selectedFileVersionId = getProp(match, 'params.fileVersionId', currentFileVersionId); - } + let selectedFileVersionId = currentFileVersionId; + if (routerDisabled && internalSidebarNavigation) { + // Use fileVersionId from internal navigation when router is disabled + selectedFileVersionId = getProp(internalSidebarNavigation, 'fileVersionId', currentFileVersionId); + } else { + // Use router-based approach when router is enabled + const match = getAnnotationsMatchPath(location); + selectedFileVersionId = getProp(match, 'params.fileVersionId', currentFileVersionId); + }Please also fix the navigation conditional block at lines 147-156.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sidebar/ActivitySidebar.js(9 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
src/elements/content-sidebar/ActivitySidebar.js
[error] 80-80: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 1130-1130: Expected an expression but instead found '}'.
Expected an expression here.
(parse)
[error] 1137-1137: expected } but instead found ;
Remove ;
(parse)
[error] 1138-1138: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 1140-1140: expected } but instead found const
Remove const
(parse)
[error] 1142-1142: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 1147-1147: expected } but instead found if
Remove if
(parse)
[error] 1153-1153: expected } but instead found ;
Remove ;
(parse)
[error] 1154-1154: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 1155-1155: expected } but instead found ;
Remove ;
(parse)
[error] 1156-1156: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 1139-1139: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
[error] 1198-1198: expected } but instead found const
Remove const
(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 (5)
src/elements/content-sidebar/ActivitySidebar.js (5)
91-92: LGTM: Well-structured props for internal navigation.The new props are properly typed as optional and clearly named to support the internal navigation mechanism when router is disabled.
Also applies to: 101-101
555-555: LGTM: Conditional navigation logic correctly implemented.The updateReplies method properly handles navigation based on router availability. When router is disabled, it uses the internal navigation handler; otherwise, it falls back to the standard history.replace approach.
Also applies to: 575-584
555-590: Implementation approach for optional router is well-designed.Once syntax errors are resolved, the conditional navigation logic provides a clean separation between router-enabled and router-disabled modes. The approach:
- Uses internal navigation handler when router is disabled
- Falls back to standard history methods when router is enabled
- Handles file version ID selection appropriately for each mode
- Maintains backward compatibility
This design aligns well with the PR objective of enabling optional router usage.
Also applies to: 1117-1160
1-1362: Ignore syntax error warning: ActivitySidebar.js is syntactically valid.Node’s
-csyntax check passed without errors, and no unclosed braces or broken conditionals were found. The file parses correctly as JavaScript/Flow—there’s no need to address syntax issues.Likely an incorrect or invalid review comment.
554-590: Test coverage for ActivitySidebar navigation logic verifiedAll paths through the new navigation logic are exercised:
- ActivitySidebar.rtl.test.js includes
- A
describe('handleAnnotationSelect()')block for router-enabled behavior- A
describe('handleAnnotationSelect() – Router Disabled')block- A
describe('updateReplies()')block for router-enabled behavior- A
describe('updateReplies() – Router Disabled')block- File version ID selection is tested via cases where
getAnnotationsMatchPathreturns valid, undefined, or null IDs- Edge cases such as missing handlers, null file versions, and unchanged feed items are covered
- SidebarToggle, SidebarNavButton, SidebarNavTablist, and AddTaskButton tests similarly cover both
routerDisabledmodesNo additional tests are needed.
There was a problem hiding this comment.
new file can be tsx and doesn't need the .rtl. name
There was a problem hiding this comment.
nvm i see the name is to not conflict with the existing file
Enabled an optional Router usage in ActivitySidebar component.
Converted related tests to RTL and added coverage for new logic.
Summary by CodeRabbit
New Features
Bug Fixes
Tests