feat(preview-optimizations): modernize Versions sidebar #4381
Conversation
WalkthroughThe pull request implements a preview modernization feature flag for the versions sidebar components. VersionsItem, VersionsItemBadge, VersionsItemActions, and VersionsList are updated to conditionally render modernized UI elements when the feature is enabled. Tests are migrated from enzyme to React Testing Library. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/elements/content-sidebar/versions/VersionsItemBadge.js (1)
8-8:isCurrentstyling hook is implemented cleanlyUsing
classNamesto gatebcs-VersionsItemBadge--currenton the optionalisCurrentprop is straightforward and keeps the badge’s aria‑label behavior unchanged. If there’s no use case for omittingversionNumber, you might keep it required inPropsto avoid silently rendering an undefined value, but that’s optional.Also applies to: 14-16, 18-27
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/elements/content-sidebar/versions/__tests__/__snapshots__/VersionsItemActions.test.js.snapis excluded by!**/*.snapsrc/elements/content-sidebar/versions/__tests__/__snapshots__/VersionsItemBadge.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
src/elements/content-sidebar/versions/VersionsItem.js(1 hunks)src/elements/content-sidebar/versions/VersionsItem.scss(1 hunks)src/elements/content-sidebar/versions/VersionsItemActions.js(3 hunks)src/elements/content-sidebar/versions/VersionsItemBadge.js(1 hunks)src/elements/content-sidebar/versions/VersionsList.js(3 hunks)src/elements/content-sidebar/versions/__tests__/VersionsItemActions.test.js(1 hunks)src/elements/content-sidebar/versions/__tests__/VersionsItemBadge.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements 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.
📚 Learning: 2025-06-11T16:30:10.431Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements 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.
Applied to files:
src/elements/content-sidebar/versions/VersionsItem.jssrc/elements/content-sidebar/versions/__tests__/VersionsItemBadge.test.jssrc/elements/content-sidebar/versions/VersionsItemActions.jssrc/elements/content-sidebar/versions/VersionsItemBadge.jssrc/elements/content-sidebar/versions/VersionsList.js
📚 Learning: 2025-08-27T17:03:48.322Z
Learnt from: bxie-box
Repo: box/box-ui-elements PR: 4250
File: src/features/classification/applied-by-ai-classification-reason/__tests__/AppliedByAiClassificationReason.test.tsx:44-51
Timestamp: 2025-08-27T17:03:48.322Z
Learning: In test files for bxie-box, prefer using queryByTestId over getByTestId when testing for expected elements that should always exist, as null access serves as validation for regressions or unexpected changes rather than masking issues with defensive assertions.
Applied to files:
src/elements/content-sidebar/versions/__tests__/VersionsItemBadge.test.jssrc/elements/content-sidebar/versions/__tests__/VersionsItemActions.test.js
📚 Learning: 2025-09-15T17:04:28.279Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js:259-260
Timestamp: 2025-09-15T17:04:28.279Z
Learning: When wrapping enzyme mount() tests with IntlProvider for class-based components, use wrapper.find('ComponentName') to access the actual component instance and state, since the wrapper becomes the provider instead of the component.
Applied to files:
src/elements/content-sidebar/versions/__tests__/VersionsItemBadge.test.js
📚 Learning: 2025-07-11T14:43:02.677Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Applied to files:
src/elements/content-sidebar/versions/VersionsItemActions.js
📚 Learning: 2025-08-15T14:42:01.840Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4227
File: src/elements/common/sub-header/SubHeader.tsx:19-19
Timestamp: 2025-08-15T14:42:01.840Z
Learning: In SubHeader.tsx, the bulkItemActionMenuProps prop is intentionally required (not optional) because there will always be a default "Download" action available, ensuring the prop is never undefined in actual usage.
Applied to files:
src/elements/content-sidebar/versions/VersionsItemActions.js
📚 Learning: 2025-08-18T20:57:20.993Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4227
File: src/elements/common/sub-header/SubHeaderRight.tsx:0-0
Timestamp: 2025-08-18T20:57:20.993Z
Learning: In the SubHeaderRight component's bulk action menu (src/elements/common/sub-header/SubHeaderRight.tsx), the BulkItemActionMenu trigger should only be shown when there are actual bulk actions available (bulkItemActions exists and has length > 0), not when the actions array is empty.
Applied to files:
src/elements/content-sidebar/versions/VersionsItemActions.jssrc/elements/content-sidebar/versions/__tests__/VersionsItemActions.test.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements 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.
Applied to files:
src/elements/content-sidebar/versions/VersionsList.js
🧬 Code graph analysis (4)
src/elements/content-sidebar/versions/VersionsItem.js (1)
src/elements/content-sidebar/versions/VersionsItemBadge.js (1)
VersionsItemBadge(18-31)
src/elements/content-sidebar/versions/VersionsItemActions.js (2)
src/elements/content-sidebar/versions/VersionsList.js (1)
isPreviewModernizationEnabled(27-27)src/elements/common/feature-checking/hooks.js (1)
useFeatureConfig(6-9)
src/elements/content-sidebar/versions/VersionsList.js (2)
src/elements/content-sidebar/versions/VersionsItemActions.js (1)
isPreviewModernizationEnabled(65-65)src/elements/common/feature-checking/hooks.js (1)
useFeatureConfig(6-9)
src/elements/content-sidebar/versions/__tests__/VersionsItemActions.test.js (1)
src/elements/content-sidebar/versions/VersionsItemActions.js (1)
VersionsItemActions(50-159)
🪛 Biome (2.1.2)
src/elements/content-sidebar/versions/VersionsItemActions.js
[error] 102-102: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/versions/VersionsItemBadge.js
[error] 11-16: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 18-18: 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)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (5)
src/elements/content-sidebar/versions/VersionsItem.scss (1)
31-36: Modernized toggle positioning looks consistentNew
.bcs-VersionsItemActions-toggle--modernizedis scoped correctly under.bcs-VersionsItemand mirrors the legacy toggle behavior with design‑token spacing, so this looks good.src/elements/content-sidebar/versions/VersionsItem.js (1)
132-134: ForwardingisCurrentinto the badge is correctThreading
isCurrentintoVersionsItemBadgematches the new badge API and keeps the current‑version styling in sync with the rest of the item.src/elements/content-sidebar/versions/VersionsList.js (1)
9-11: Feature‑flagged modernized list class is wired correctlyUsing
useFeatureConfig('previewModernization')andclassNamesto conditionally addbcs-VersionsList--modernizedkeeps behavior identical when the flag is off and opt‑in when on; this integration looks solid.Also applies to: 27-28, 52-55
src/elements/content-sidebar/versions/VersionsItemActions.js (1)
8-11: Feature‑flagged modernized toggle preserves behavior and a11yThe
previewModernizationflag cleanly switches between the legacyPlainButtonand the new BlueprintIconButtonwhile keeping the analytics attributes and accessible name (versionActionToggle) consistent, so the user‑visible and tracking behavior look preserved.Also applies to: 65-67, 80-105
src/elements/content-sidebar/versions/__tests__/VersionsItemBadge.test.js (1)
2-2: Badge class assertions target the wrong element
screen.getByText('V1')will match the inner<span>rendered byFormattedMessage, not the outer<div>that actually has thebcs-VersionsItemBadge/--currentclasses, so the class checks will fail even when the component is correct.Consider targeting the labeled container instead, e.g.:
- test.each` + test.each` isCurrent | shouldHaveCurrentClass ${true} | ${true} ${false} | ${false} ${undefined} | ${false} `( 'should apply current class correctly when isCurrent is $isCurrent', ({ isCurrent, shouldHaveCurrentClass }) => { renderComponent({ versionNumber: '1', isCurrent }); - const badge = screen.getByText('V1'); + const badge = screen.getByLabelText('Version number 1'); expect(badge).toHaveClass('bcs-VersionsItemBadge'); if (shouldHaveCurrentClass) { expect(badge).toHaveClass('bcs-VersionsItemBadge--current'); } else { expect(badge).not.toHaveClass('bcs-VersionsItemBadge--current'); } }, );Also applies to: 6-6, 9-39
⛔ Skipped due to learnings
Learnt from: bxie-box Repo: box/box-ui-elements PR: 4250 File: src/features/classification/applied-by-ai-classification-reason/__tests__/AppliedByAiClassificationReason.test.tsx:44-51 Timestamp: 2025-08-27T17:03:48.322Z Learning: In test files for bxie-box, prefer using queryByTestId over getByTestId when testing for expected elements that should always exist, as null access serves as validation for regressions or unexpected changes rather than masking issues with defensive assertions.
src/elements/content-sidebar/versions/__tests__/VersionsItemActions.test.js
Show resolved
Hide resolved
src/elements/content-sidebar/versions/__tests__/VersionsItemBadge.test.js
Show resolved
Hide resolved
Merge Queue Status✅ The pull request has been merged This pull request spent 10 minutes 57 seconds in the queue, including 10 minutes 47 seconds running CI. Required conditions to merge
|
3 participants
Finished product in combination with EUA:
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.