Skip to content

Conversation

rafalmaksymiuk
Copy link
Contributor

@rafalmaksymiuk rafalmaksymiuk commented Jul 31, 2025

Making Router optional in VersionsSidebarContainer.
Covered new logic with tests.

Summary by CodeRabbit

  • New Features

    • Added support for internal sidebar navigation, allowing navigation updates without using browser routing when enabled.
    • Introduced new options to control navigation behavior in the versions sidebar.
  • Tests

    • Enhanced test coverage for version deletion scenarios, including both router-enabled and internal navigation modes.
    • Updated tests to use React Testing Library for improved reliability and coverage.

@rafalmaksymiuk rafalmaksymiuk requested review from a team as code owners July 31, 2025 10:43
Copy link
Contributor

coderabbitai bot commented Jul 31, 2025

Walkthrough

This update introduces optional internal sidebar navigation to VersionsSidebarContainer, controlled by new props and handler types. The navigation logic in updateVersion is now conditional, supporting both router-based and internal state-driven navigation. Related props and types are propagated to child components, and tests are refactored for broader coverage and modern practices.

Changes

Cohort / File(s) Change Summary
VersionsSidebarContainer logic and types
src/elements/content-sidebar/versions/VersionsSidebarContainer.js
Adds optional props (internalSidebarNavigation, internalSidebarNavigationHandler, routerDisabled) and extends the Props type. Updates updateVersion to conditionally use the internal navigation handler or React Router. Passes new props to child components. Imports new navigation types.
VersionsSidebarContainer tests
src/elements/content-sidebar/versions/__tests__/VersionsSidebarContainer.test.js
Refactors tests to use React Testing Library with React Router integration. Expands coverage for navigation logic, especially internal navigation and router-disabled scenarios. Adds parameterized tests for delete actions, mocks API calls and components, and introduces render helpers for router-enabled and router-disabled modes. No changes to exported entities.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant VersionsSidebarContainer
    participant InternalNavHandler
    participant ReactRouter

    User->>VersionsSidebarContainer: Triggers version update (e.g., delete)
    alt routerDisabled & internalSidebarNavigationHandler provided
        VersionsSidebarContainer->>InternalNavHandler: Call handler with new state
    else router enabled
        VersionsSidebarContainer->>ReactRouter: history.push(new version route)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

ready-to-merge

Suggested reviewers

  • jankowiakdawid
  • kajarosz

Poem

A sidebar’s path now splits in two,
Internal routes or router’s queue.
With props and tests both fresh and bright,
Navigation’s handled left or right.
Rabbits hop with glee and cheer,
For flexible sidebars are finally here!
🐇✨

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4669f2b and f65c880.

📒 Files selected for processing (2)
  • src/elements/content-sidebar/versions/VersionsSidebarContainer.js (4 hunks)
  • src/elements/content-sidebar/versions/__tests__/VersionsSidebarContainer.test.js (2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 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.
📚 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/versions/__tests__/VersionsSidebarContainer.test.js
  • src/elements/content-sidebar/versions/VersionsSidebarContainer.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/versions/__tests__/VersionsSidebarContainer.test.js
  • src/elements/content-sidebar/versions/VersionsSidebarContainer.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/versions/__tests__/VersionsSidebarContainer.test.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/versions/VersionsSidebarContainer.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/versions/VersionsSidebarContainer.js
🧬 Code Graph Analysis (2)
src/elements/content-sidebar/versions/__tests__/VersionsSidebarContainer.test.js (5)
src/elements/content-sidebar/versions/VersionsSidebarContainer.js (2)
  • versions (201-201)
  • selectedVersionId (168-168)
src/elements/content-sidebar/versions/__tests__/VersionsSidebarAPI.test.js (1)
  • versionsAPI (11-20)
src/elements/content-sidebar/versions/VersionsSidebar.js (2)
  • props (132-132)
  • VersionsSidebar (131-143)
src/elements/content-sidebar/versions/__tests__/VersionsSidebar.test.js (1)
  • renderComponent (49-55)
src/elements/content-sidebar/__tests__/SidebarToggle.test.js (1)
  • mockInternalSidebarNavigationHandler (34-34)
src/elements/content-sidebar/versions/VersionsSidebarContainer.js (3)
src/elements/common/types/SidebarNavigation.ts (2)
  • InternalSidebarNavigation (49-51)
  • InternalSidebarNavigationHandler (55-55)
src/elements/content-sidebar/versions/__tests__/VersionsSidebarContainer.test.js (1)
  • history (61-61)
src/elements/content-sidebar/versions/VersionsSidebar.js (1)
  • VersionsSidebar (131-143)
🪛 Biome (2.1.2)
src/elements/content-sidebar/versions/VersionsSidebarContainer.js

[error] 29-29: 'import { type x ident }' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 30-30: 'import { type x ident }' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 31-31: 'import { type x ident }' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 279-279: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.

TypeScript only syntax

(parse)


[error] 320-329: Illegal return statement outside of a function

(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 (10)
src/elements/content-sidebar/versions/VersionsSidebarContainer.js (4)

27-32: LGTM! Flow type imports are correctly added.

The new type imports for InternalSidebarNavigation and InternalSidebarNavigationHandler are properly structured using Flow syntax, which is consistent with the codebase's use of Flow type annotations (as indicated by the @flow directive).


40-42: LGTM! Props are well-designed for backward compatibility.

The new optional props (internalSidebarNavigation, internalSidebarNavigationHandler, routerDisabled) are properly typed and maintain backward compatibility while enabling the new router-less navigation functionality.

Also applies to: 52-52


274-289: LGTM! Conditional navigation logic is well-implemented.

The updateVersion method correctly implements the dual navigation approach:

  • Uses internal navigation handler when routerDisabled is true and handler is provided
  • Preserves existing navigation state while updating/removing versionId appropriately
  • Falls back to React Router's history.push for backward compatibility

The implementation addresses the previous review feedback about handling versionId internally.


310-346: LGTM! Props are consistently passed to child components.

The render method correctly:

  • Destructures all new navigation-related props
  • Passes them to both StaticVersionsSidebar and VersionsSidebar components
  • Maintains existing prop spreading pattern for clean code
src/elements/content-sidebar/versions/__tests__/VersionsSidebarContainer.test.js (6)

3-29: LGTM! Modern testing approach with proper mocking.

The migration to React Testing Library with explicit mocking of dependencies is a good modernization:

  • Uses React Testing Library for more realistic DOM testing
  • Properly mocks VersionsSidebarAPI and VersionsSidebar for controlled testing
  • Includes Router integration for navigation testing

37-56: LGTM! Comprehensive API mocking setup.

The mock setup correctly provides all necessary API methods with jest.fn() and properly mocks the VersionsSidebarAPI constructor to return the mocked methods. This provides good isolation for testing component behavior.


63-73: LGTM! Well-designed render helpers for both navigation modes.

The render helpers cleanly separate testing scenarios:

  • renderComponent: Tests router-enabled navigation with proper Router wrapper
  • renderComponentWithoutRouter: Tests router-disabled navigation
  • Both allow prop overrides and provide necessary defaults

85-101: LGTM! Properly modernized async test.

The componentDidMount test correctly uses React Testing Library patterns:

  • act() for component rendering
  • waitFor() for async operations
  • Maintains original test coverage while using modern testing practices

104-172: LGTM! Comprehensive parameterized tests for router-enabled deletion.

The delete tests excellently cover both scenarios:

  • Non-selected version deletion: Verifies no navigation occurs
  • Selected version deletion: Verifies navigation to current version
  • Proper mocking of child components to trigger actions
  • Thorough verification of API calls, callbacks, and navigation behavior
  • Good use of parameterized testing for scenario coverage

175-251: LGTM! Excellent coverage for router-disabled navigation.

The router-disabled delete tests perfectly complement the router-enabled tests:

  • Same scenario coverage: Non-selected vs selected version deletion
  • Proper internal navigation testing: Verifies internalSidebarNavigationHandler calls
  • Realistic mock data: Internal sidebar navigation object with appropriate properties
  • Consistent test structure: Mirrors router-enabled tests for maintainability

The test coverage ensures both navigation modes work correctly.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

coderabbitai bot commented Jul 31, 2025

Note

Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it.


Generating unit tests... This may take up to 20 minutes.

Copy link

@fsahinx fsahinx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

Copy link
Contributor

@jmalinna jmalinna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mergify mergify bot merged commit 0f46866 into box:master Aug 4, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants