feat(preview-modernization): Update Sidebar Nav icons and tooltips #4373
Conversation
WalkthroughAdds a feature-flagged previewModernization path across sidebar components that conditionally swaps legacy tooltips/icons for Blueprint tooltips and modern icons, propagates the flag to children, and adds CSS modifiers to scale SVG icons on hover/focus. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Sidebar Component
participant FeatureFlag as useFeatureConfig (previewModernization)
participant Tooltip as Tooltip system (BPTooltip / Legacy Tooltip)
participant Icons as Icon provider (legacy / modern)
participant UI as Rendered UI
Component->>FeatureFlag: read previewModernization
FeatureFlag-->>Component: true / false
alt previewModernization = true
Component->>Tooltip: choose BPTooltip and wrap element (span workaround)
Component->>Icons: select modern icons (BoxAiLogo24, filled icons)
Component->>UI: add modernized CSS classes (bcs/bdl-SidebarNav--modernized)
Tooltip->>UI: attach Blueprint tooltip behavior
Icons->>UI: render modern icons with SIDEBAR_TAB_ICON_PROPS
else previewModernization = false
Component->>Tooltip: use legacy Tooltip with existing props
Component->>Icons: render legacy icons
Component->>UI: render existing classes and behavior
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 0
🧹 Nitpick comments (3)
src/elements/content-sidebar/SidebarNavSignButton.scss (1)
14-14: Optional: Remove semicolon after comment.The semicolon after the inline comment is unusual and redundant in CSS.
Apply this diff if you'd like to clean it up:
- margin-top: 2px; // for visual alignment; + margin-top: 2px; // for visual alignmentsrc/elements/content-sidebar/SidebarNavSignButton.tsx (1)
4-7: Modernization and tooltip behavior look good; handlearia-expandedmore robustlyThe feature-flagged BPTooltip path, reusing
PlainButtonand skipping the inner tooltip when FTUX is active, matches the legacy behavior while integrating Blueprint tooltips. One small improvement:isDropdownOpenonly treats'aria-expanded'as the string'true', but callers may pass a boolean.You can make this more robust without changing behavior:
- const renderButtonWithTooltip = () => { - const isDropdownOpen = rest['aria-expanded'] === 'true'; + const renderButtonWithTooltip = () => { + const ariaExpanded = rest['aria-expanded']; + const isDropdownOpen = ariaExpanded === true || ariaExpanded === 'true';This keeps the color logic correct regardless of whether
aria-expandedis passed as a boolean or a string.Also applies to: 13-13, 37-38, 53-80, 82-97, 99-107
src/elements/content-sidebar/SidebarNavButton.js (1)
11-18: Dynamic tooltip selection andisActivecloning are sound; avoid leakingisActiveto DOM elementsUsing
TooltipComponentto switch between BPTooltip and the legacy Tooltip based onisPreviewModernizationEnabled, with matchingtooltipProps, keeps behavior consistent across the routerDisabled and Route paths. Cloningchildrento injectisActiveis a nice way to let icon wrappers react to active state, but doing this unconditionally can push anisActiveattribute down to raw DOM elements if a child ultimately renders, for example, a<span>or<svg>and blindly spreads props.You can keep the feature while avoiding unknown DOM attributes by only injecting
isActivefor non-DOM React components:- // Clone children and pass isActive prop - const childrenWithProps = React.isValidElement(children) - ? React.cloneElement(children, { isActive: isActiveValue }) - : children; + // Clone children and pass isActive prop only for non-DOM components + const childrenWithProps = + React.isValidElement(children) && typeof children.type !== 'string' + ? React.cloneElement(children, { isActive: isActiveValue }) + : children;Apply the same pattern in both the
routerDisabledandRoutebranches.Also, the
import typeusage forInternalSidebarNavigation,InternalSidebarNavigationHandler, andViewTypeValuesis valid Flow syntax for this file (@flowheader); Biome’s “TypeScript-only” warning here is a tooling configuration issue rather than a code problem.Also applies to: 34-35, 56-61, 68-75, 92-99, 93-95, 144-151, 145-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/components/sidebar-toggle-button/SidebarToggleButton.js(2 hunks)src/components/sidebar-toggle-button/SidebarToggleButton.scss(1 hunks)src/elements/content-sidebar/SidebarNav.js(8 hunks)src/elements/content-sidebar/SidebarNavButton.js(6 hunks)src/elements/content-sidebar/SidebarNavButton.scss(3 hunks)src/elements/content-sidebar/SidebarNavSignButton.scss(1 hunks)src/elements/content-sidebar/SidebarNavSignButton.tsx(4 hunks)src/elements/content-sidebar/_mixins.scss(2 hunks)src/elements/content-sidebar/additional-tabs/AdditionalTab.scss(1 hunks)src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js(2 hunks)src/elements/content-sidebar/additional-tabs/AdditionalTabs.js(3 hunks)src/elements/content-sidebar/additional-tabs/AdditionalTabs.scss(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-09-03T18:10:42.760Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarNav.js:195-206
Timestamp: 2025-09-03T18:10:42.760Z
Learning: In src/elements/content-sidebar/SidebarNav.js, the maintainer wants users to be able to override data-* attributes (data-resin-target, data-target-id, data-testid) through navButtonProps for custom tabs. The spread of {...navButtonProps} should come after the data-* attributes to allow overriding, not before them.
Applied to files:
src/elements/content-sidebar/additional-tabs/AdditionalTabs.jssrc/elements/content-sidebar/additional-tabs/AdditionalTab.scsssrc/elements/content-sidebar/SidebarNav.jssrc/elements/content-sidebar/SidebarNavSignButton.tsxsrc/elements/content-sidebar/SidebarNavButton.jssrc/components/sidebar-toggle-button/SidebarToggleButton.jssrc/elements/content-sidebar/additional-tabs/AdditionalTabs.scsssrc/components/sidebar-toggle-button/SidebarToggleButton.scsssrc/elements/content-sidebar/SidebarNavSignButton.scsssrc/elements/content-sidebar/SidebarNavButton.scss
📚 Learning: 2025-09-03T18:30:44.447Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarPanels.js:0-0
Timestamp: 2025-09-03T18:30:44.447Z
Learning: In the CustomSidebarPanel type, the component field is required (React.ComponentType<any>), so runtime checks for component existence are unnecessary since Flow will catch missing components at compile time. User fpan225 prefers to rely on the type system rather than adding redundant runtime checks.
Applied to files:
src/elements/content-sidebar/additional-tabs/AdditionalTabs.jssrc/elements/content-sidebar/SidebarNavSignButton.tsxsrc/elements/content-sidebar/SidebarNavButton.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/SidebarNav.jssrc/elements/content-sidebar/SidebarNavSignButton.tsxsrc/elements/content-sidebar/SidebarNavButton.jssrc/components/sidebar-toggle-button/SidebarToggleButton.js
📚 Learning: 2025-09-03T18:10:29.467Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarNav.js:94-106
Timestamp: 2025-09-03T18:10:29.467Z
Learning: In the ContentSidebar CustomSidebarPanel API, the navButtonProps spread should come after explicit data-* attributes to allow users to override analytics tracking attributes like data-resin-target, data-target-id, and data-testid when needed. This is intentional API design for flexibility.
Applied to files:
src/elements/content-sidebar/SidebarNav.jssrc/elements/content-sidebar/SidebarNavSignButton.tsxsrc/elements/content-sidebar/SidebarNavButton.jssrc/elements/content-sidebar/SidebarNavButton.scss
📚 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/SidebarNav.jssrc/components/sidebar-toggle-button/SidebarToggleButton.js
📚 Learning: 2025-06-25T13:09:54.538Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 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/SidebarNav.js
📚 Learning: 2025-06-25T13:09:45.168Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements 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.
Applied to files:
src/elements/content-sidebar/SidebarNav.js
📚 Learning: 2025-06-25T13:10:19.128Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 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/SidebarNav.js
📚 Learning: 2025-09-09T21:43:34.180Z
Learnt from: bxie-box
Repo: box/box-ui-elements PR: 4271
File: src/features/classification/README.md:67-67
Timestamp: 2025-09-09T21:43:34.180Z
Learning: In the Classification component's aiClassificationReason prop, TooltipProvider is only required when the citations array is empty (shows tooltip UI), not when citations are present (shows card on hover UI).
Applied to files:
src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.jssrc/elements/content-sidebar/SidebarNavSignButton.tsx
📚 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/SidebarNavButton.js
🧬 Code graph analysis (6)
src/elements/content-sidebar/additional-tabs/AdditionalTabs.js (3)
src/elements/content-sidebar/SidebarNav.js (1)
isPreviewModernizationEnabled(96-96)src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
isPreviewModernizationEnabled(35-35)src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (1)
isPreviewModernizationEnabled(23-23)
src/elements/content-sidebar/SidebarNav.js (7)
src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
isPreviewModernizationEnabled(35-35)src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (1)
isPreviewModernizationEnabled(23-23)src/elements/common/feature-checking/hooks.js (1)
useFeatureConfig(6-9)src/elements/content-sidebar/messages.js (1)
messages(9-115)src/elements/content-sidebar/SidebarNavButton.js (1)
SidebarNavButton(37-37)src/elements/content-sidebar/Sidebar.js (3)
hasActivity(320-320)hasDetails(321-321)hasSkills(323-323)src/elements/common/interactionTargets.js (2)
SIDEBAR_NAV_TARGETS(2-11)SIDEBAR_NAV_TARGETS(2-11)
src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (1)
src/elements/common/feature-checking/hooks.js (1)
useFeatureConfig(6-9)
src/elements/content-sidebar/SidebarNavSignButton.tsx (2)
src/elements/common/feature-checking/hooks.js (1)
useFeatureConfig(6-9)src/elements/content-sidebar/messages.js (1)
messages(9-115)
src/elements/content-sidebar/SidebarNavButton.js (1)
src/utils/dom.js (1)
isLeftClick(79-81)
src/components/sidebar-toggle-button/SidebarToggleButton.js (3)
src/elements/content-sidebar/SidebarNav.js (1)
isPreviewModernizationEnabled(96-96)src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (1)
isPreviewModernizationEnabled(23-23)src/elements/common/feature-checking/hooks.js (1)
useFeatureConfig(6-9)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarNav.js
[error] 110-110: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 121-121: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 132-132: 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/SidebarNavButton.js
[error] 14-18: 'import type' 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 (19)
src/elements/content-sidebar/SidebarNavSignButton.scss (1)
17-23: LGTM!The modernized SVG scaling implementation is consistent with the broader modernization pattern across the sidebar components.
src/elements/content-sidebar/additional-tabs/AdditionalTab.scss (2)
15-23: LGTM!The modernized styling correctly handles both SVG and IMG elements for additional tab icons, ensuring consistent behavior regardless of icon type.
26-26: LGTM!The explicit duration format (0.5s) improves readability and consistency.
src/elements/content-sidebar/_mixins.scss (2)
17-17: LGTM!The explicit opacity format (0.5) improves readability and aligns with modern CSS conventions.
30-33: LGTM!The new mixin provides standardized 20px sizing for modernized icons, creating visual consistency across the modernized sidebar navigation.
src/components/sidebar-toggle-button/SidebarToggleButton.scss (1)
33-45: LGTM!The modernized styling correctly handles both legacy (
.is-disabled) and current (.bdl-is-disabled) class patterns, ensuring backward compatibility while providing consistent SVG scaling across all toggle button states.src/elements/content-sidebar/additional-tabs/AdditionalTabs.scss (2)
1-2: LGTM!The import is necessary for the
bdl-SidebarNavIconModernizedmixin used in the modernized modifier block.
7-12: LGTM!The modernized modifier correctly applies the standardized 20px icon sizing to both regular and placeholder tab icons when modernization is enabled.
src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (3)
8-8: LGTM!The imports are necessary for the Blueprint tooltip integration and feature flag checking.
Also applies to: 11-11
23-23: LGTM!The feature flag check follows the established pattern used across other modernized components.
26-33: The span wrapper is architecturally justified for this wrapper component.AdditionalTabTooltip is a generic wrapper component that accepts children as a prop, not the owner/controller of the button. The span wrapper is necessary because BPTooltip requires a DOM element to attach to, and this wrapper cannot assume its children (which come from consumers) will forward refs. This pattern is consistent with similar modernization code elsewhere in the codebase (e.g., SidebarToggleButton.js). The suggestion to update the button component to forward refs should be addressed at the call sites where buttons are passed as children, not here.
Likely an incorrect or invalid review comment.
src/elements/content-sidebar/additional-tabs/AdditionalTabs.js (3)
8-8: LGTM!The classNames utility is appropriate for conditional CSS class application.
17-17: LGTM!The optional boolean prop is correctly typed for the feature flag.
58-66: LGTM!The conditional class application correctly enables modernized styling through CSS cascade, without needing to pass the prop to child components.
src/elements/content-sidebar/SidebarNavButton.scss (2)
27-27: LGTM!The single-quote format normalizes the attribute selector style and maintains consistency with the new modernized block.
48-57: LGTM!The modernized styling correctly applies SVG scaling to hover, focus, and selected states using semantic ARIA attributes, consistent with the broader modernization pattern.
src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
6-6: BPTooltip integration and feature-flag path look solidThe previewModernization branch correctly introduces BPTooltip with an opposite-side tooltip, applies the modernized CSS modifier class, and falls back cleanly to the legacy Tooltip path when the flag is disabled. No issues spotted with the hook usage or button rendering.
Also applies to: 11-11, 35-42, 51-64
src/elements/content-sidebar/SidebarNav.js (2)
11-21: Icon wrappers and Flow annotations align with modernization goalsCentralizing icon sizing in
SIDEBAR_TAB_ICON_PROPSand using the filled vs outline Blueprint icons in theActivityIconWrapper,DetailsIconWrapper, andMetadataIconWrapperbased onisPreviewModernizationEnabledandisActivegives clear, consistent visual states. The inline parameter type annotations (({ isActive }: { isActive?: boolean })) are valid Flow syntax in this codebase; the Biome “TypeScript-only” parse errors for these lines are false positives and should be handled via linter configuration rather than code changes. Based on learningsAlso applies to: 47-50, 96-142
143-149: Modernized nav container, buttons, and overflow wiring look consistentApplying
bcs-SidebarNav--modernizedandbcs-SidebarNav-overflow--modernizedbased onisPreviewModernizationEnabled, while threading the same flag into eachSidebarNavButtonandAdditionalTabs, keeps the legacy behavior intact and cleanly scopes modern styles behind the feature flag. Data attributes and existing tab wiring remain unchanged, so analytics and routing behavior should preserve current behavior.Also applies to: 159-237, 255-266
src/elements/content-sidebar/additional-tabs/AdditionalTabs.scss
Outdated
Show resolved
Hide resolved
ahorowitz123
force-pushed
the
sidebarnav-icons
branch
from
c133283 to
a60f67b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-sidebar/SidebarNav.js (1)
12-20: Icon modernization flow and flag propagation are cohesiveThe new
SIDEBAR_TAB_ICON_PROPSplus theActivity/Details/MetadataIconWrappercomponents cleanly switch between legacy and blueprint icons, including active‑state coloring, and thebcs-SidebarNav--modernized/bcs-SidebarNav-overflow--modernizedmodifiers give CSS a single source of truth for the modernized state. PassingisPreviewModernizationEnabledthrough toSidebarNavButtonandAdditionalTabskeeps the feature flag centralized and consistent.If you ever see perf pressure here, the small wrapper components could be hoisted outside
SidebarNavand receiveisPreviewModernizationEnabledas a prop instead of closing over it, but that’s purely an optimization, not a requirement for this change.Also applies to: 47-50, 96-147, 159-167, 181-232, 255-266
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/components/sidebar-toggle-button/SidebarToggleButton.js(2 hunks)src/components/sidebar-toggle-button/SidebarToggleButton.scss(1 hunks)src/elements/content-sidebar/SidebarNav.js(7 hunks)src/elements/content-sidebar/SidebarNavButton.js(6 hunks)src/elements/content-sidebar/SidebarNavButton.scss(3 hunks)src/elements/content-sidebar/SidebarNavSignButton.scss(1 hunks)src/elements/content-sidebar/SidebarNavSignButton.tsx(4 hunks)src/elements/content-sidebar/__tests__/SidebarNavButton.test.js(13 hunks)src/elements/content-sidebar/_mixins.scss(2 hunks)src/elements/content-sidebar/additional-tabs/AdditionalTab.scss(1 hunks)src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js(2 hunks)src/elements/content-sidebar/additional-tabs/AdditionalTabs.js(3 hunks)src/elements/content-sidebar/additional-tabs/AdditionalTabs.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/elements/content-sidebar/tests/SidebarNavButton.test.js
- src/components/sidebar-toggle-button/SidebarToggleButton.scss
- src/elements/content-sidebar/_mixins.scss
- src/elements/content-sidebar/additional-tabs/AdditionalTab.scss
- src/components/sidebar-toggle-button/SidebarToggleButton.js
- src/elements/content-sidebar/SidebarNavButton.scss
- src/elements/content-sidebar/additional-tabs/AdditionalTabs.scss
- src/elements/content-sidebar/SidebarNavSignButton.scss
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-09-09T21:43:34.180Z
Learnt from: bxie-box
Repo: box/box-ui-elements PR: 4271
File: src/features/classification/README.md:67-67
Timestamp: 2025-09-09T21:43:34.180Z
Learning: In the Classification component's aiClassificationReason prop, TooltipProvider is only required when the citations array is empty (shows tooltip UI), not when citations are present (shows card on hover UI).
Applied to files:
src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.jssrc/elements/content-sidebar/SidebarNavSignButton.tsx
📚 Learning: 2025-09-03T18:10:42.760Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarNav.js:195-206
Timestamp: 2025-09-03T18:10:42.760Z
Learning: In src/elements/content-sidebar/SidebarNav.js, the maintainer wants users to be able to override data-* attributes (data-resin-target, data-target-id, data-testid) through navButtonProps for custom tabs. The spread of {...navButtonProps} should come after the data-* attributes to allow overriding, not before them.
Applied to files:
src/elements/content-sidebar/SidebarNavSignButton.tsxsrc/elements/content-sidebar/SidebarNav.jssrc/elements/content-sidebar/SidebarNavButton.jssrc/elements/content-sidebar/additional-tabs/AdditionalTabs.js
📚 Learning: 2025-09-03T18:10:29.467Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarNav.js:94-106
Timestamp: 2025-09-03T18:10:29.467Z
Learning: In the ContentSidebar CustomSidebarPanel API, the navButtonProps spread should come after explicit data-* attributes to allow users to override analytics tracking attributes like data-resin-target, data-target-id, and data-testid when needed. This is intentional API design for flexibility.
Applied to files:
src/elements/content-sidebar/SidebarNavSignButton.tsxsrc/elements/content-sidebar/SidebarNav.jssrc/elements/content-sidebar/SidebarNavButton.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/SidebarNavSignButton.tsxsrc/elements/content-sidebar/SidebarNav.jssrc/elements/content-sidebar/SidebarNavButton.js
📚 Learning: 2025-09-03T18:30:44.447Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarPanels.js:0-0
Timestamp: 2025-09-03T18:30:44.447Z
Learning: In the CustomSidebarPanel type, the component field is required (React.ComponentType<any>), so runtime checks for component existence are unnecessary since Flow will catch missing components at compile time. User fpan225 prefers to rely on the type system rather than adding redundant runtime checks.
Applied to files:
src/elements/content-sidebar/SidebarNavSignButton.tsxsrc/elements/content-sidebar/SidebarNav.jssrc/elements/content-sidebar/SidebarNavButton.jssrc/elements/content-sidebar/additional-tabs/AdditionalTabs.js
📚 Learning: 2025-06-25T13:09:54.538Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 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/SidebarNav.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/SidebarNav.js
📚 Learning: 2025-06-25T13:09:45.168Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements 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.
Applied to files:
src/elements/content-sidebar/SidebarNav.js
📚 Learning: 2025-06-25T13:10:19.128Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 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/SidebarNav.js
📚 Learning: 2025-09-16T14:53:03.538Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/components/form-elements/draft-js-mention-selector/index.js:4-4
Timestamp: 2025-09-16T14:53:03.538Z
Learning: The createMentionSelectorState export in src/components/form-elements/draft-js-mention-selector/index.js was confirmed to be internal-only within the box-ui-elements package, with no external references or documentation, making the breaking rename to createMentionTimestampSelectorState safe.
Applied to files:
src/elements/content-sidebar/SidebarNav.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/SidebarNavButton.js
🧬 Code graph analysis (3)
src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (3)
src/elements/content-sidebar/SidebarNav.js (1)
isPreviewModernizationEnabled(96-96)src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
isPreviewModernizationEnabled(35-35)src/elements/common/feature-checking/hooks.js (1)
useFeatureConfig(6-9)
src/elements/content-sidebar/SidebarNavButton.js (1)
src/utils/dom.js (1)
isLeftClick(79-81)
src/elements/content-sidebar/additional-tabs/AdditionalTabs.js (3)
src/elements/content-sidebar/SidebarNav.js (1)
isPreviewModernizationEnabled(96-96)src/components/sidebar-toggle-button/SidebarToggleButton.js (1)
isPreviewModernizationEnabled(35-35)src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (1)
isPreviewModernizationEnabled(23-23)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarNav.js
[error] 110-110: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 121-121: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 132-132: 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/SidebarNavButton.js
[error] 14-18: 'import type' 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 (4)
src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (1)
8-12: Non-FTUX Blueprint tooltip path is well‑gated and preserves existing behaviorThe
previewModernizationflag is wired correctly: FTUX still usesTargetedClickThroughGuideTooltip, while the non‑FTUX path cleanly switches betweenBPTooltipand the legacyTooltip. The span workaround aroundchildrenforBPTooltipis reasonable until the button is fully migrated.Also applies to: 22-33
src/elements/content-sidebar/additional-tabs/AdditionalTabs.js (1)
8-8: Preview modernization flag and BEM modifier wiring look solidAdding
isPreviewModernizationEnabledand conditionally applyingbdl-AdditionalTabs--modernizedviaclassNamesis straightforward and keeps existing loading/tab logic intact. Prop remains optional and non‑breaking.Also applies to: 17-18, 58-66
src/elements/content-sidebar/SidebarNavButton.js (1)
11-18: Unable to complete verification due to repository access failureThe review comment cannot be fully verified without access to the box/box-ui-elements repository. The approval and analysis appear sound based on the provided code snippet, but verification of call sites and children type constraints would require codebase inspection.
src/elements/content-sidebar/SidebarNavSignButton.tsx (1)
4-7: React automatically convertsaria-expandedboolean values to strings during renderingReact's JSX automatically coerces
aria-expanded={true}to the DOM attributearia-expanded="true". The ARIA specification requires the string values"true"or"false", not booleans, and React handles this conversion transparently. Therefore, the comparisonrest['aria-expanded'] === 'true'will correctly handle callers passingaria-expanded={true}because React converts it to the string"true"before the attribute is accessed.The suggested fix (checking both
=== trueand=== 'true') is not necessary and adds unnecessary complexity.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-sidebar/SidebarNav.js (1)
113-145: Consider extracting icon wrapper components outside the render function.Defining components inside the render function causes them to be recreated on every render, which may lead to unnecessary re-renders and loss of component identity.
Apply this diff to extract the wrappers:
+// Icon wrapper components that receive isActive prop from SidebarNavButton +const ActivityIconWrapper = ({ isActive, isPreviewModernizationEnabled }: { isActive?: boolean, isPreviewModernizationEnabled: boolean }) => { + if (!isPreviewModernizationEnabled) { + return <IconChatRound className="bcs-SidebarNav-icon" />; + } + return isActive ? ( + <CommentIconFilled {...SIDEBAR_TAB_ICON_PROPS} color={IconIconBlue} /> + ) : ( + <CommentIcon {...SIDEBAR_TAB_ICON_PROPS} /> + ); +}; + +const DetailsIconWrapper = ({ isActive, isPreviewModernizationEnabled }: { isActive?: boolean, isPreviewModernizationEnabled: boolean }) => { + if (!isPreviewModernizationEnabled) { + return <IconDocInfo className="bcs-SidebarNav-icon" />; + } + return isActive ? ( + <InformationCircleIconFilled {...SIDEBAR_TAB_ICON_PROPS} color={IconIconBlue} /> + ) : ( + <InformationCircleIcon {...SIDEBAR_TAB_ICON_PROPS} /> + ); +}; + +const MetadataIconWrapper = ({ isActive, isPreviewModernizationEnabled }: { isActive?: boolean, isPreviewModernizationEnabled: boolean }) => { + if (!isPreviewModernizationEnabled) { + return <IconMetadataThick className="bcs-SidebarNav-icon" />; + } + return isActive ? ( + <MetadataIconFilled {...SIDEBAR_TAB_ICON_PROPS} color={IconIconBlue} /> + ) : ( + <MetadataIcon {...SIDEBAR_TAB_ICON_PROPS} /> + ); +}; + const SidebarNav = ({ additionalTabs, elementId, fileId, hasActivity, hasAdditionalTabs, hasBoxAI, hasDetails, hasMetadata, hasSkills, hasDocGen = false, internalSidebarNavigation, internalSidebarNavigationHandler, intl, isOpen, onNavigate, onPanelChange = noop, routerDisabled, signSidebarProps, }: Props) => { const { enabled: hasBoxSign } = signSidebarProps || {}; const { disabledTooltip: boxAIDisabledTooltip, showOnlyNavButton: showOnlyBoxAINavButton } = useFeatureConfig('boxai.sidebar'); const { enabled: isPreviewModernizationEnabled } = useFeatureConfig('previewModernization'); const { focusPrompt } = usePromptFocus('.be.bcs'); const handleSidebarNavButtonClick = (sidebarview: string) => { onPanelChange(sidebarview, false); // If the Box AI sidebar is enabled, focus the Box AI sidebar prompt if (sidebarview === SIDEBAR_VIEW_BOXAI) { focusPrompt(); } }; - // Icon wrapper components that receive isActive prop from SidebarNavButton - const ActivityIconWrapper = ({ isActive }: { isActive?: boolean }) => { - if (!isPreviewModernizationEnabled) { - return <IconChatRound className="bcs-SidebarNav-icon" />; - } - return isActive ? ( - <CommentIconFilled {...SIDEBAR_TAB_ICON_PROPS} color={IconIconBlue} /> - ) : ( - <CommentIcon {...SIDEBAR_TAB_ICON_PROPS} /> - ); - }; - - const DetailsIconWrapper = ({ isActive }: { isActive?: boolean }) => { - if (!isPreviewModernizationEnabled) { - return <IconDocInfo className="bcs-SidebarNav-icon" />; - } - return isActive ? ( - <InformationCircleIconFilled {...SIDEBAR_TAB_ICON_PROPS} color={IconIconBlue} /> - ) : ( - <InformationCircleIcon {...SIDEBAR_TAB_ICON_PROPS} /> - ); - }; - - const MetadataIconWrapper = ({ isActive }: { isActive?: boolean }) => { - if (!isPreviewModernizationEnabled) { - return <IconMetadataThick className="bcs-SidebarNav-icon" />; - } - return isActive ? ( - <MetadataIconFilled {...SIDEBAR_TAB_ICON_PROPS} color={IconIconBlue} /> - ) : ( - <MetadataIcon {...SIDEBAR_TAB_ICON_PROPS} /> - ); - };Then update the wrapper usages to pass the flag:
- <ActivityIconWrapper /> + <ActivityIconWrapper isPreviewModernizationEnabled={isPreviewModernizationEnabled} />(Apply similar changes for DetailsIconWrapper and MetadataIconWrapper)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
flow-typed/npm/@box/blueprint-web-assets_vx.x.x.js(3 hunks)src/elements/content-sidebar/SidebarNav.js(7 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-06-25T13:09:54.538Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 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:
flow-typed/npm/@box/blueprint-web-assets_vx.x.x.jssrc/elements/content-sidebar/SidebarNav.js
📚 Learning: 2025-06-25T13:10:19.128Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 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:
flow-typed/npm/@box/blueprint-web-assets_vx.x.x.jssrc/elements/content-sidebar/SidebarNav.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:
flow-typed/npm/@box/blueprint-web-assets_vx.x.x.jssrc/elements/content-sidebar/SidebarNav.js
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 4224
File: package.json:296-297
Timestamp: 2025-08-12T18:04:17.698Z
Learning: In the box-ui-elements project, the team is comfortable with raising peerDependency minimum versions when upgrading blueprint-web packages, even if it's a breaking change for consumers.
Applied to files:
flow-typed/npm/@box/blueprint-web-assets_vx.x.x.js
📚 Learning: 2025-08-21T19:19:44.584Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4237
File: src/api/ZipDownload.ts:6-8
Timestamp: 2025-08-21T19:19:44.584Z
Learning: For Box TypeScript SDK integrations, jpan-box prefers deep imports (e.g., from box-typescript-sdk-gen/lib/schemas/...generated.d.ts.js) over defining local types, citing that Box v2 APIs don't typically have breaking changes, making the deep import approach more viable despite potential brittleness.
Applied to files:
flow-typed/npm/@box/blueprint-web-assets_vx.x.x.js
📚 Learning: 2025-06-25T13:09:45.168Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements 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.
Applied to files:
flow-typed/npm/@box/blueprint-web-assets_vx.x.x.jssrc/elements/content-sidebar/SidebarNav.js
📚 Learning: 2025-09-03T18:10:42.760Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarNav.js:195-206
Timestamp: 2025-09-03T18:10:42.760Z
Learning: In src/elements/content-sidebar/SidebarNav.js, the maintainer wants users to be able to override data-* attributes (data-resin-target, data-target-id, data-testid) through navButtonProps for custom tabs. The spread of {...navButtonProps} should come after the data-* attributes to allow overriding, not before them.
Applied to files:
src/elements/content-sidebar/SidebarNav.js
📚 Learning: 2025-09-03T18:10:29.467Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarNav.js:94-106
Timestamp: 2025-09-03T18:10:29.467Z
Learning: In the ContentSidebar CustomSidebarPanel API, the navButtonProps spread should come after explicit data-* attributes to allow users to override analytics tracking attributes like data-resin-target, data-target-id, and data-testid when needed. This is intentional API design for flexibility.
Applied to files:
src/elements/content-sidebar/SidebarNav.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/SidebarNav.js
📚 Learning: 2025-09-03T18:30:44.447Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarPanels.js:0-0
Timestamp: 2025-09-03T18:30:44.447Z
Learning: In the CustomSidebarPanel type, the component field is required (React.ComponentType<any>), so runtime checks for component existence are unnecessary since Flow will catch missing components at compile time. User fpan225 prefers to rely on the type system rather than adding redundant runtime checks.
Applied to files:
src/elements/content-sidebar/SidebarNav.js
🧬 Code graph analysis (2)
flow-typed/npm/@box/blueprint-web-assets_vx.x.x.js (1)
src/elements/common/annotator-context/types.ts (1)
Metadata(72-75)
src/elements/content-sidebar/SidebarNav.js (3)
src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (1)
isPreviewModernizationEnabled(23-23)src/elements/content-sidebar/SidebarNavButton.js (1)
SidebarNavButton(37-37)src/elements/common/interactionTargets.js (2)
SIDEBAR_NAV_TARGETS(2-11)SIDEBAR_NAV_TARGETS(2-11)
🪛 Biome (2.1.2)
flow-typed/npm/@box/blueprint-web-assets_vx.x.x.js
[error] 13-14: The 'declare' modifier can only be used in TypeScript files.
(parse)
src/elements/content-sidebar/SidebarNav.js
[error] 114-114: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 125-125: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 136-136: 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 (6)
src/elements/content-sidebar/SidebarNav.js (5)
12-23: LGTM! Clean icon import additions.The new icon imports from
@box/blueprint-web-assetsare well-organized, separating Medium (outline) and MediumFilled variants for the modernization feature.
51-54: LGTM! Consistent icon sizing.The
SIDEBAR_TAB_ICON_PROPSconstant ensures uniform dimensions across all modernized icons.
148-152: LGTM! Consistent feature flag propagation.The
isPreviewModernizationEnabledflag is properly propagated to all relevant components (SidebarNavButton,AdditionalTabs) and appropriate CSS modifiers are applied to the container elements.Also applies to: 165-165, 187-187, 200-200, 213-213, 226-226, 240-240, 260-271
178-182: Verify the intentional icon size difference between legacy and modern BoxAI icons.The legacy BoxAI icon uses
Size6, while the modernized version usesBoxAiLogo24withSIDEBAR_TAB_ICON_PROPS. Please confirm whether the size difference between these implementations is intentional or if they should use consistent sizing.
195-195: Verify that SidebarNavButton properly injects the isActive prop to icon wrapper children.The icon wrappers (ActivityIconWrapper, DetailsIconWrapper, MetadataIconWrapper) expect an
isActiveprop, but it's not explicitly passed in the JSX. Ensure thatSidebarNavButtonclones its children and injects this prop correctly.Also applies to: 208-208, 234-234
flow-typed/npm/@box/blueprint-web-assets_vx.x.x.js (1)
10-18: LGTM! Flow type definitions correctly match the new icon imports.The added type declarations for
Comment,Metadata,InformationCircle(in both Medium and MediumFilled variants),BoxAiLogo24, andIconIconBlueproperly support the modernization changes inSidebarNav.js.Note: The static analysis hint about
declarebeing TypeScript-only is a false positive. Based on learnings,declare moduleis valid Flow syntax for type definitions in.flow.jsfiles.Also applies to: 46-46, 66-66
|
@tjuanitas can we get one more review here? I addressed the PR comments and also did some refactoring and fixed flow issues |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-sidebar/SidebarNav.js (1)
216-255: Note: Skills and DocGen not using modernized icons.Unlike Activity, Details, and Metadata, the Skills and DocGen tabs still use legacy icons (
IconMagicWand,DocGenIcon) even though they receive theisPreviewModernizationEnabledprop. This is likely intentional if these tabs aren't included in the current modernization phase.If Skills and DocGen should be modernized in this PR, consider adding wrapper components similar to the pattern used for Activity, Details, and Metadata.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sidebar/SidebarNav.js(7 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-09-03T18:10:42.760Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarNav.js:195-206
Timestamp: 2025-09-03T18:10:42.760Z
Learning: In src/elements/content-sidebar/SidebarNav.js, the maintainer wants users to be able to override data-* attributes (data-resin-target, data-target-id, data-testid) through navButtonProps for custom tabs. The spread of {...navButtonProps} should come after the data-* attributes to allow overriding, not before them.
Applied to files:
src/elements/content-sidebar/SidebarNav.js
📚 Learning: 2025-09-03T18:10:29.467Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarNav.js:94-106
Timestamp: 2025-09-03T18:10:29.467Z
Learning: In the ContentSidebar CustomSidebarPanel API, the navButtonProps spread should come after explicit data-* attributes to allow users to override analytics tracking attributes like data-resin-target, data-target-id, and data-testid when needed. This is intentional API design for flexibility.
Applied to files:
src/elements/content-sidebar/SidebarNav.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/SidebarNav.js
📚 Learning: 2025-06-25T13:09:54.538Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 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/SidebarNav.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/SidebarNav.js
📚 Learning: 2025-06-25T13:09:45.168Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements 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.
Applied to files:
src/elements/content-sidebar/SidebarNav.js
📚 Learning: 2025-06-25T13:10:19.128Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 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/SidebarNav.js
📚 Learning: 2025-09-03T18:30:44.447Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarPanels.js:0-0
Timestamp: 2025-09-03T18:30:44.447Z
Learning: In the CustomSidebarPanel type, the component field is required (React.ComponentType<any>), so runtime checks for component existence are unnecessary since Flow will catch missing components at compile time. User fpan225 prefers to rely on the type system rather than adding redundant runtime checks.
Applied to files:
src/elements/content-sidebar/SidebarNav.js
🧬 Code graph analysis (1)
src/elements/content-sidebar/SidebarNav.js (4)
src/elements/content-sidebar/additional-tabs/AdditionalTabTooltip.js (1)
isPreviewModernizationEnabled(23-23)src/elements/content-sidebar/messages.js (1)
messages(9-115)src/elements/content-sidebar/SidebarNavButton.js (1)
SidebarNavButton(37-37)src/elements/common/interactionTargets.js (2)
SIDEBAR_NAV_TARGETS(2-11)SIDEBAR_NAV_TARGETS(2-11)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarNav.js
[error] 55-59: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 62-62: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 73-73: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 84-84: 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 (6)
src/elements/content-sidebar/SidebarNav.js (6)
12-23: LGTM! Clean modernization imports.The Blueprint icon imports are well-organized, with both outline and filled variants to support active/inactive states. The token imports ensure consistent sizing and colors across the modernized UI.
51-54: LGTM! Good centralization of icon properties.Using a constant for icon sizing ensures consistency across all sidebar tabs and makes future adjustments easier.
153-158: LGTM! Proper BEM modifier for modernization.The conditional
bcs-SidebarNav--modernizedclass follows BEM conventions and enables feature-specific styling.
168-189: LGTM! Clean conditional rendering for BoxAI logo.The direct conditional rendering is appropriate here since BoxAI doesn't require active/inactive state toggling like the other tabs. The modernized
BoxAiLogo24maintains the proper sizing viaSIDEBAR_TAB_ICON_PROPS.
264-276: LGTM! Consistent modernization propagation.The overflow container receives the appropriate BEM modifier class, and
isPreviewModernizationEnabledis correctly propagated toAdditionalTabs, ensuring consistent modernization across all sidebar tabs including custom ones.
56-93:isActiveprop flow to IconWrapper components requires verification.The IconWrapper components are designed to toggle between filled and outline icons based on the
isActiveprop. However, the provided code snippet does not show howisActiveis passed when these components are rendered.Verify:
- Whether
SidebarNavButtonclones children and passesisActiveas a prop- Whether
isActiveflows through context, render props, or another mechanism- Actual usage at lines 200, 213, and 239 to confirm if
isActiveis explicitly passedIf
isActiveis not reaching the IconWrapper components through any mechanism, the filled icon variants will never render on active tabs, breaking visual feedback.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/elements/content-sidebar/__tests__/SidebarNavSignButton.test.tsx (1)
11-11: Consider adding test coverage for theisDropdownOpenprop.The test wrapper now passes
isDropdownOpen={false}to all test cases. While this satisfies the new required prop, consider adding tests that explicitly verify behavior whenisDropdownOpen={true}, particularly for the modernization path where the icon color changes based on this prop.Example test to add:
test('should render SignIcon with blue color when dropdown is open and modernization enabled', () => { // Mock feature config to enable modernization const wrapper = getWrapper({ isDropdownOpen: true }); // Assert SignIcon color is blue when dropdown is open // (implementation depends on how feature flags are mocked in tests) });src/elements/content-sidebar/SidebarNavSignButton.tsx (1)
20-20: Required prop is acceptable, but consider making it optional.The
isDropdownOpen: booleanprop is required, which is fine since the parentSidebarNavSignalways provides it. However, you could make this more flexible by making it optional with a default value:export type Props = PlainButtonProps & { blockedReason?: string; - isDropdownOpen: boolean; + isDropdownOpen?: boolean; intl: IntlShape; targetingApi?: {Then in the function signature:
-export function SidebarNavSignButton({ blockedReason, intl, isDropdownOpen, targetingApi, ...rest }: Props) { +export function SidebarNavSignButton({ blockedReason, intl, isDropdownOpen = false, targetingApi, ...rest }: Props) {This would make the component more resilient to usage variations, though it's not essential if all current call sites provide the prop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/elements/content-sidebar/SidebarNavSign.tsx(1 hunks)src/elements/content-sidebar/SidebarNavSignButton.tsx(4 hunks)src/elements/content-sidebar/__tests__/SidebarNavSignButton.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-03T18:10:42.760Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarNav.js:195-206
Timestamp: 2025-09-03T18:10:42.760Z
Learning: In src/elements/content-sidebar/SidebarNav.js, the maintainer wants users to be able to override data-* attributes (data-resin-target, data-target-id, data-testid) through navButtonProps for custom tabs. The spread of {...navButtonProps} should come after the data-* attributes to allow overriding, not before them.
Applied to files:
src/elements/content-sidebar/__tests__/SidebarNavSignButton.test.tsxsrc/elements/content-sidebar/SidebarNavSignButton.tsxsrc/elements/content-sidebar/SidebarNavSign.tsx
📚 Learning: 2025-09-03T18:10:29.467Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarNav.js:94-106
Timestamp: 2025-09-03T18:10:29.467Z
Learning: In the ContentSidebar CustomSidebarPanel API, the navButtonProps spread should come after explicit data-* attributes to allow users to override analytics tracking attributes like data-resin-target, data-target-id, and data-testid when needed. This is intentional API design for flexibility.
Applied to files:
src/elements/content-sidebar/__tests__/SidebarNavSignButton.test.tsxsrc/elements/content-sidebar/SidebarNavSignButton.tsxsrc/elements/content-sidebar/SidebarNavSign.tsx
📚 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/SidebarNavSignButton.tsx
📚 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/SidebarNavSignButton.tsx
🧬 Code graph analysis (2)
src/elements/content-sidebar/__tests__/SidebarNavSignButton.test.tsx (1)
src/elements/content-sidebar/SidebarNavSignButton.tsx (1)
SidebarNavSignButton(32-109)
src/elements/content-sidebar/SidebarNavSign.tsx (1)
src/elements/content-sidebar/SidebarNavSignButton.tsx (1)
SidebarNavSignButton(32-109)
⏰ 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/SidebarNavSign.tsx (2)
37-45: LGTM! Clean state management.The dropdown state management is straightforward and correct. The handlers properly toggle the state, which is then passed to
SidebarNavSignButtonto control the icon color in the modernization path.
48-58: LGTM! Dropdown configuration and state propagation look correct.The
DropdownMenunow explicitly setsisResponsive,constrainToWindow, andisRightAligned, and the callbacks correctly manage the dropdown state. TheisDropdownOpenprop is properly passed toSidebarNavSignButtonto enable the icon color change when the dropdown is active.src/elements/content-sidebar/SidebarNavSignButton.tsx (3)
4-6: LGTM! Imports are clean and all used.The new Blueprint and feature-config imports are all utilized in the modernization rendering path.
Also applies to: 13-13
54-96: LGTM! Dual rendering paths are well-structured.The
renderButtonWithTooltipfunction cleanly separates modernization and legacy rendering paths:
- Modern path: Uses Blueprint
SignIconwith dynamic color based onisDropdownOpen, and wraps withBPTooltipwhen not targeted- Legacy path: Uses existing
BoxSign28/Sign16icons with legacyTooltipThe span wrapper workaround for Blueprint tooltip compatibility is well-documented and acceptable as a temporary measure.
Note on past review comment: There's a previous comment noting something "feels odd" without specific line numbers. If this refers to the span wrapper (lines 76), it's a documented workaround that should be removed when buttons are migrated to Blueprint—the comment clearly states this intent.
98-108: LGTM! FtuxTooltip wrapper correctly preserved.The
FtuxTooltipwrapper correctly wraps both the modernized and legacy rendering paths, preserving the first-time user experience guidance regardless of the feature flag state.
Merge Queue Status
🚫 The pull request has left the queue (rule: This pull request spent 11 minutes 36 seconds in the queue, with no time running CI. Required conditions to merge
ReasonThe merge conditions cannot be satisfied due to failing checks HintYou may have to fix your CI before adding the pull request to the queue again. |
Merge Queue Status
✅ The pull request has been merged This pull request spent 18 seconds in the queue, with no time running CI. Required conditions to merge
|
4 participants
Summary by CodeRabbit
New Features
Style
Tests
✏️ Tip: You can customize this high-level summary in your review settings.