feat(content-sidebar): implement for custom sidebar panels #4326
Conversation
|
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/elements/content-sidebar/SidebarNav.js (1)
82-105: Make that Box AI focus check respect custom paths, fool!Right now the click handler only fires
focusPrompt()whensidebarview === SIDEBAR_VIEW_BOXAI. With the new API, the Box AI panel can ship with an alternatepath, so the button will stop focusing the prompt even though the tab ID is stillboxai. I pity the fool who ships a regression like that! Key the check off the actual Box AI tab metadata so custom paths still trigger the focus.- if (sidebarview === SIDEBAR_VIEW_BOXAI) { + if (boxAiTab && sidebarview === boxAiTab.path) { focusPrompt(); }src/elements/content-sidebar/SidebarPanels.js (1)
146-156: Remove unused Box AI sidebar cache and ref, fool
I pity the fool who lets dead code linger:boxAiSidebarCacheis never accessed, and theboxAISidebarref + refresh block are redundant withcustomSidebars. Drop the state field (lines 146–156), remove the ref & itsrefresh()(lines 209–212), and update SidebarPanels.test.js to remove 'boxAISidebar' from the sidebar list and assertions.
🧹 Nitpick comments (5)
src/elements/content-sidebar/SidebarPanels.js (4)
327-360: Drop redundant component existence checks; Flow’s got your back, fool!
CustomSidebarPanel.componentis required by type. Remove!CustomPanelComponentguard and the ternary. Cleaner, less noise.Apply:
- if (isDisabled || !CustomPanelComponent) { + if (isDisabled) { return null; } return ( <Route exact key={customPanelId} path={`/${customPanelPath}`} render={() => { this.handlePanelRender(customPanelPath); - return CustomPanelComponent ? ( - <CustomPanelComponent - elementId={elementId} - key={file.id} - fileExtension={file.extension} - hasSidebarInitialized={isInitialized} - ref={this.getCustomSidebarRef(customPanelId)} - /> - ) : null; + return ( + <CustomPanelComponent + elementId={elementId} + key={file.id} + fileExtension={file.extension} + hasSidebarInitialized={isInitialized} + ref={this.getCustomSidebarRef(customPanelId)} + /> + ); }} /> );Based on learnings
186-196: Refs won’t attach to function components without forwardRef — don’t get played, sucka!You pass
ref={this.getCustomSidebarRef(customPanelId)}to arbitrary custom components. Unless they useReact.forwardRef(and ideallyuseImperativeHandleto exposerefresh()),ref.currentstays null and your custom refresh loop won’t hit them. Add a note to theCustomSidebarPanelcontract/docs requiring forwardRef for refresh support, or switch to a callback/imperative prop.Also applies to: 349-355
237-254: Panel ordering logic looks right; Box AI included when not default — solid work, fool.Else-branch now includes all custom panels (including Box AI). Nice fix versus earlier exclusion. One caveat: if a custom panel path collides with a built-in (e.g., “metadata”), it can override eligibility/order. Add a reserved-name guard or validate uniqueness of
panel.path.Example guard (conceptual):
- Reserved: {docgen, skills, activity, details, metadata}
- Warn or skip any custom with a reserved path
292-308: Avoid key collisions in eligibility map, jive turkey!Keys for custom eligibility are paths that can overwrite built-ins if they collide. Consider validating custom
pathagainst reserved names before merging, or namespace custom keys to prevent accidental overrides.src/elements/content-sidebar/__tests__/SidebarPanels.test.js (1)
663-819: Consider a refresh test for custom panels — show me the receipts, sucka!Add a test where a custom panel uses
forwardRefto exposerefresh(), mount, callinstance.refresh(), and assert your custom ref’srefreshis called via thecustomSidebarsloop.I can draft the test with a
forwardRefcustom component and verifyrefreshinvocation. Want me to add it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/elements/content-sidebar/ContentSidebar.js(4 hunks)src/elements/content-sidebar/Sidebar.js(6 hunks)src/elements/content-sidebar/SidebarNav.js(4 hunks)src/elements/content-sidebar/SidebarPanels.js(8 hunks)src/elements/content-sidebar/__tests__/SidebarNav.test.js(4 hunks)src/elements/content-sidebar/__tests__/SidebarPanels.test.js(19 hunks)src/elements/content-sidebar/flowTypes.js(2 hunks)src/elements/content-sidebar/stories/BoxAISideBar.mdx(0 hunks)src/elements/content-sidebar/stories/BoxAISidebar.stories.tsx(0 hunks)src/elements/content-sidebar/stories/tests/BoxAISidebar-visual.stories.tsx(0 hunks)src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx(2 hunks)
💤 Files with no reviewable changes (3)
- src/elements/content-sidebar/stories/BoxAISidebar.stories.tsx
- src/elements/content-sidebar/stories/tests/BoxAISidebar-visual.stories.tsx
- src/elements/content-sidebar/stories/BoxAISideBar.mdx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-03T18:30:44.447Z
Learnt from: fpan225
PR: box/box-ui-elements#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/SidebarPanels.jssrc/elements/content-sidebar/Sidebar.jssrc/elements/content-sidebar/flowTypes.jssrc/elements/content-sidebar/ContentSidebar.jssrc/elements/content-sidebar/SidebarNav.js
📚 Learning: 2025-09-23T21:14:20.232Z
Learnt from: fpan225
PR: box/box-ui-elements#4239
File: src/elements/content-sidebar/SidebarPanels.js:0-0
Timestamp: 2025-09-23T21:14:20.232Z
Learning: User fpan225 clarified that Box AI is included in customPanels if it exists, but the current getPanelOrder function logic in the else branch still excludes Box AI from the returned order when shouldBoxAIBeDefaultPanel is false, since otherCustomPanelPaths specifically filters out Box AI panels.
Applied to files:
src/elements/content-sidebar/SidebarPanels.jssrc/elements/content-sidebar/__tests__/SidebarPanels.test.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sidebar/Sidebar.jssrc/elements/content-sidebar/flowTypes.jssrc/elements/content-sidebar/ContentSidebar.js
📚 Learning: 2025-09-03T18:24:37.905Z
Learnt from: fpan225
PR: box/box-ui-elements#4239
File: src/elements/content-sidebar/SidebarPanels.js:0-0
Timestamp: 2025-09-03T18:24:37.905Z
Learning: User fpan225 prefers to keep the original `customPanels` variable name rather than normalizing it to a different variable name (like `panels`) for better code readability. They use `if (hasCustomPanels && customPanels)` pattern to satisfy Flow type checking instead of creating a normalized array.
Applied to files:
src/elements/content-sidebar/__tests__/SidebarPanels.test.js
📚 Learning: 2025-07-11T14:43:02.677Z
Learnt from: jpan-box
PR: box/box-ui-elements#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
🧬 Code graph analysis (4)
src/elements/content-sidebar/SidebarPanels.js (2)
src/elements/content-sidebar/ContentSidebar.js (4)
Props(226-226)Props(275-275)Props(320-320)Props(352-385)src/elements/content-sidebar/Sidebar.js (4)
Props(294-319)hasSkills(324-324)hasActivity(321-321)hasMetadata(323-323)
src/elements/content-sidebar/__tests__/SidebarNav.test.js (1)
src/elements/content-sidebar/SidebarNav.js (1)
boxAiTab(90-90)
src/elements/content-sidebar/__tests__/SidebarPanels.test.js (1)
src/elements/content-sidebar/Sidebar.js (1)
hasBoxAI(326-328)
src/elements/content-sidebar/SidebarNav.js (4)
src/elements/content-sidebar/ContentSidebar.js (4)
Props(226-226)Props(275-275)Props(320-320)Props(352-385)src/elements/content-sidebar/Sidebar.js (5)
Props(294-319)hasActivity(321-321)hasDetails(322-322)hasSkills(324-324)hasMetadata(323-323)src/elements/content-sidebar/SidebarNavButton.js (1)
SidebarNavButton(32-32)src/elements/common/interactionTargets.js (2)
SIDEBAR_NAV_TARGETS(2-11)SIDEBAR_NAV_TARGETS(2-11)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarPanels.js
[error] 44-44: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 136-136: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 186-186: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 186-186: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 237-237: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 237-237: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 237-237: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 237-237: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 310-310: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/Sidebar.js
[error] 31-31: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 32-32: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 33-33: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 34-34: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 35-35: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 37-37: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/flowTypes.js
[error] 45-46: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/ContentSidebar.js
[error] 49-49: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/SidebarNav.js
[error] 32-32: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 33-33: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 35-35: '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). (3)
- GitHub Check: lint_test_build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
fpan225
force-pushed
the
custom-sidebar-panels
branch
from
59c10df to
df9a123
Compare
fpan225
changed the title
fpan225
force-pushed
the
custom-sidebar-panels
branch
from
df9a123 to
595bd11
Compare
WalkthroughAdds support for consumer-provided custom sidebar panels via a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ContentSidebar
participant Sidebar
participant SidebarNav
participant SidebarPanels
User->>ContentSidebar: mount(props { customSidebarPanels? })
ContentSidebar->>Sidebar: render(customSidebarPanels)
Sidebar->>SidebarNav: render(customTabs = customSidebarPanels, hasNativeBoxAISidebar)
Sidebar->>SidebarPanels: render(customPanels = customSidebarPanels, hasNativeBoxAISidebar)
SidebarNav->>SidebarNav: compute visibleTabs(hasNativeBoxAISidebar, customTabs)
SidebarPanels->>SidebarPanels: compute panelOrder(defaults, customPanels, nativeBoxAI)
User->>SidebarNav: click tab
SidebarNav->>SidebarPanels: request showPanel(panelId)
SidebarPanels-->>User: render selected panel content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/elements/content-sidebar/SidebarNav.js`:
- Around line 202-213: Fix the Box AI button attributes by using the actual tab
id in the data-target-id (replace the literal `$boxAI` with boxAiTab.id so it
renders like SidebarNavButton-{boxAiTab.id}) and move the spread
{...boxAiTab.navButtonProps} to after the explicit data-* props (data-target-id
and data-resin-target) so consumers can override those attributes; update the
SidebarNavButton usage where hasNativeBoxAISidebar and boxAiTab are used to
apply these changes.
🧹 Nitpick comments (1)
src/elements/content-sidebar/SidebarPanels.js (1)
392-412: Consider removing redundant component check.Line 392 already guards against
!CustomPanelComponentby returningnull. The ternary check on line 403 (CustomPanelComponent ? ... : null) is redundant since execution only reaches this point whenCustomPanelComponentis truthy.Based on learnings, the
componentfield is required in theCustomSidebarPaneltype, so Flow will catch missing components at compile time.Suggested simplification
return CustomPanelComponent ? ( - <CustomPanelComponent + return ( + <CustomPanelComponent elementId={elementId} file={file} fileId={fileId} fileExtension={file.extension} hasSidebarInitialized={isInitialized} ref={this.getCustomSidebarRef(customPanelId)} /> - ) : null; + );
| !hasNativeBoxAISidebar && boxAiTab && ( | ||
| <SidebarNavButton | ||
| key={boxAiTab.id} | ||
| isPreviewModernizationEnabled={isPreviewModernizationEnabled} | ||
| data-target-id={`SidebarNavButton-$boxAI`} | ||
| data-testid={`sidebar${boxAiTab.id}`} | ||
| {...boxAiTab.navButtonProps} | ||
| data-resin-target={SIDEBAR_NAV_TARGETS.BOXAI} | ||
| isDisabled={boxAiTab.isDisabled} | ||
| onClick={handleSidebarNavButtonClick} | ||
| sidebarView={boxAiTab.path} | ||
| tooltip={boxAiTab.title ?? boxAiTab.id} |
There was a problem hiding this comment.
Fix custom Box AI data attributes and override order.
data-target-id currently renders as SidebarNavButton-$boxAI, and data-resin-target is set after the spread so custom overrides won’t work. This can break analytics labeling for consumers.
✅ Suggested fix
- data-target-id={`SidebarNavButton-$boxAI`}
- data-testid={`sidebar${boxAiTab.id}`}
- {...boxAiTab.navButtonProps}
- data-resin-target={SIDEBAR_NAV_TARGETS.BOXAI}
+ data-resin-target={SIDEBAR_NAV_TARGETS.BOXAI}
+ data-target-id={`SidebarNavButton-${SIDEBAR_VIEW_BOXAI}`}
+ data-testid={`sidebar${boxAiTab.id}`}
+ {...boxAiTab.navButtonProps}Based on learnings, keep navButtonProps after explicit data-* attributes to allow overrides.
🤖 Prompt for AI Agents
In `@src/elements/content-sidebar/SidebarNav.js` around lines 202 - 213, Fix the
Box AI button attributes by using the actual tab id in the data-target-id
(replace the literal `$boxAI` with boxAiTab.id so it renders like
SidebarNavButton-{boxAiTab.id}) and move the spread {...boxAiTab.navButtonProps}
to after the explicit data-* props (data-target-id and data-resin-target) so
consumers can override those attributes; update the SidebarNavButton usage where
hasNativeBoxAISidebar and boxAiTab are used to apply these changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/elements/content-sidebar/SidebarPanels.js`:
- Around line 311-333: panelsEligibility currently marks SIDEBAR_VIEW_BOXAI
eligible using hasNativeBoxAISidebar even when showOnlyBoxAINavButton prevents
showing the panel; update the mapping to use the computed
canShowBoxAISidebarPanel (or combine hasNativeBoxAISidebar &&
!showOnlyBoxAINavButton) for SIDEBAR_VIEW_BOXAI so the eligibility reflects the
actual visibility logic used elsewhere and prevents redirects to /boxai when the
panel is hidden; adjust the entry keyed by SIDEBAR_VIEW_BOXAI in
panelsEligibility accordingly.
♻️ Duplicate comments (2)
src/elements/content-sidebar/SidebarNav.js (1)
203-210: Fix custom Box AI data attributes and allow overrides.Line 206 hardcodes
$boxAI, and Line 208 spreadsnavButtonPropsbeforedata-resin-target, which blocks overrides. Align with the custom tab ordering and use the real id.✅ Suggested fix
- data-target-id={`SidebarNavButton-$boxAI`} - data-testid={`sidebar${boxAiTab.id}`} - {...boxAiTab.navButtonProps} - data-resin-target={SIDEBAR_NAV_TARGETS.BOXAI} + data-resin-target={SIDEBAR_NAV_TARGETS.BOXAI} + data-target-id={`SidebarNavButton-${boxAiTab.id}`} + data-testid={`sidebar${boxAiTab.id}`} + {...boxAiTab.navButtonProps}Based on learnings, keep
navButtonPropsafter explicitdata-*attributes to allow overrides.src/elements/content-sidebar/__tests__/SidebarPanels.test.js (1)
671-686: Use rendered testids for custom panel absence checks.
CustomPanel[id="custom1"]doesn’t exist, so these assertions are ineffective. Use the actual rendereddata-testidvalues.✅ Suggested fix
- expect(wrapper.exists('CustomPanel[id="custom1"]')).toBe(false); - expect(wrapper.exists('CustomPanel[id="custom2"]')).toBe(false); + expect(wrapper.find('div[data-testid="custom1-sidebar"]')).toHaveLength(0); + expect(wrapper.find('div[data-testid="custom2-sidebar"]')).toHaveLength(0);
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/elements/content-sidebar/SidebarPanels.js (1)
313-347: Prevent empty/disabled custom panels from triggering a redirect loop.
hasCustomPanelsonly checks array length. If all custom panels are disabled/ineligible, the early-return doesn’t fire, and the fallback redirect can’t find an eligible panel, causing redirects to/with nothing renderable. Consider deriving a “has eligible custom panels” flag fromcustomPanelEligibilityand using that in the early-return guard.💡 Suggested fix
- const hasCustomPanels = customPanels && customPanels.length > 0; + const hasCustomPanels = customPanels && customPanels.length > 0; const customPanelEligibility = {}; if (hasCustomPanels) { // $FlowFixMe: customPanels is checked for existence in hasCustomPanels customPanels.forEach(({ id, path, isDisabled }) => { const isBoxAICustomPanel = id === SIDEBAR_VIEW_BOXAI; const isEligible = isBoxAICustomPanel ? !canShowBoxAISidebarPanel && !isDisabled : !isDisabled; customPanelEligibility[path] = isEligible; }); } + const hasEligibleCustomPanels = Object.values(customPanelEligibility).some(Boolean); if ( !isOpen || (!hasActivity && !hasNativeBoxAISidebar && !hasDetails && !hasMetadata && !hasSkills && !hasDocGen && !hasVersions && - !hasCustomPanels) + !hasEligibleCustomPanels) ) { return null; }
♻️ Duplicate comments (1)
src/elements/content-sidebar/__tests__/SidebarPanels.test.js (1)
805-808: Use DOM testids instead of a non-existentCustomPanelselector.
CustomPanel[id="custom1"]doesn’t exist in the render tree. Assert against the actualdata-testidoutput.✅ Suggested fix
- expect(wrapper.exists('CustomPanel[id="custom1"]')).toBe(false); - expect(wrapper.exists('CustomPanel[id="custom2"]')).toBe(false); + expect(wrapper.find('div[data-testid="custom1-sidebar"]')).toHaveLength(0); + expect(wrapper.find('div[data-testid="custom2-sidebar"]')).toHaveLength(0);
🧹 Nitpick comments (2)
src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx (1)
89-125: Strengthen the “default panel” assertion.
Line 121-124 only checks presence; add anaria-selectedcheck to confirm it is actually the default.♻️ Suggested tweak to the play assertion
- await waitFor(() => expect(canvas.getByTestId('sidebarcustomPanel')).toBeInTheDocument(), { timeout: 5000 }); + await waitFor( + () => { + const customPanelButton = canvas.getByTestId('sidebarcustomPanel'); + expect(customPanelButton).toBeInTheDocument(); + expect(customPanelButton).toHaveAttribute('aria-selected', 'true'); + }, + { timeout: 5000 }, + );src/elements/content-sidebar/SidebarPanels.js (1)
392-413: Remove redundantCustomPanelComponentexistence checks.
CustomSidebarPanel.componentis required, so the!CustomPanelComponentguard and the ternary render are redundant and can mask contract violations. Simplify to render the component directly and keep theisDisabledguard only.Based on learnings, prefer relying on the Flow-required `component` contract here.♻️ Suggested simplification
- if (isDisabled || !CustomPanelComponent) { + if (isDisabled) { return null; } return ( <Route exact key={customPanelId} path={`/${customPanelPath}`} render={() => { this.handlePanelRender(customPanelPath); - return CustomPanelComponent ? ( - <CustomPanelComponent - elementId={elementId} - file={file} - fileId={fileId} - fileExtension={file.extension} - hasSidebarInitialized={isInitialized} - ref={this.getCustomSidebarRef(customPanelId)} - /> - ) : null; + return ( + <CustomPanelComponent + elementId={elementId} + file={file} + fileId={fileId} + fileExtension={file.extension} + hasSidebarInitialized={isInitialized} + ref={this.getCustomSidebarRef(customPanelId)} + /> + ); }} /> );
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/elements/content-sidebar/SidebarPanels.js (1)
276-347: Prevent redirecting to “/” when no panels are eligible.
IfshowOnlyBoxAINavButtonis true (or all custom panels are disabled) and no other panels are eligible,redirectstays empty and the router navigates to/. Guard the “no eligible panels” case so the sidebar doesn’t bounce to root.🐛 Suggested fix
- const showDefaultPanel: boolean = !!(defaultPanel && panelsEligibility[defaultPanel]); + const showDefaultPanel: boolean = !!(defaultPanel && panelsEligibility[defaultPanel]); + const hasEligiblePanels: boolean = Object.values(panelsEligibility).some(Boolean); - if ( - !isOpen || - (!hasActivity && - !hasNativeBoxAISidebar && - !hasDetails && - !hasMetadata && - !hasSkills && - !hasDocGen && - !hasVersions && - !hasCustomPanels) - ) { + if (!isOpen || (!hasEligiblePanels && !hasVersions)) { return null; } ... - const firstEligiblePanel = panelOrder.find(panel => panelsEligibility[panel]); - if (firstEligiblePanel) { - redirect = firstEligiblePanel; - } + const firstEligiblePanel = panelOrder.find(panel => panelsEligibility[panel]); + if (firstEligiblePanel) { + redirect = firstEligiblePanel; + } else { + return null; + }Also applies to: 579-589
🧹 Nitpick comments (1)
src/elements/content-sidebar/SidebarPanels.js (1)
377-414: Remove redundantCustomPanelComponentexistence checks.
componentis required by theCustomSidebarPaneltype, so the runtime null checks add noise without real protection.♻️ Proposed cleanup
- if (isDisabled || !CustomPanelComponent) { + if (isDisabled) { return null; } ... - return CustomPanelComponent ? ( - <CustomPanelComponent - elementId={elementId} - file={file} - fileId={fileId} - fileExtension={file.extension} - hasSidebarInitialized={isInitialized} - ref={this.getCustomSidebarRef(customPanelId)} - /> - ) : null; + return ( + <CustomPanelComponent + elementId={elementId} + file={file} + fileId={fileId} + fileExtension={file.extension} + hasSidebarInitialized={isInitialized} + ref={this.getCustomSidebarRef(customPanelId)} + /> + );Based on learnings, the
componentfield is required by Flow and doesn’t need runtime guards.
| {hasNav && ( | ||
| <SidebarNav | ||
| additionalTabs={additionalTabs} | ||
| customTabs={customSidebarPanels} |
There was a problem hiding this comment.
It seems like customTabs, customPanels, and customSidebarPanels all refer to the same thing. Is it possible to align them all? I can see a future reader being confused by this.
| isDisabled={boxAiTab.isDisabled} | ||
| onClick={handleSidebarNavButtonClick} | ||
| sidebarView={boxAiTab.path} | ||
| tooltip={boxAiTab.title ?? boxAiTab.id} |
There was a problem hiding this comment.
@coderabbitai I see that we have a fallback if title does not exist. However, title is a required prop based on the type at https://github.com/box/box-ui-elements/pull/4326/changes#diff-d015620767ee46b3cee314ed8ccde92256aeabb5b2bb061adf6be8d0fda903b5R40. Can you double check if this fallback is needed?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the CustomSidebarPanel type definition from flowTypes.js
cat src/elements/content-sidebar/flowTypes.js | grep -A 20 "type CustomSidebarPanel"
Length of output: 684
@jpan-box, you're absolutely correct! Looking at the type definition in flowTypes.js, title is indeed a required field (not optional):
type CustomSidebarPanel = {
id: string,
path: string,
component: React.ComponentType<any>,
title: string, // ← Required, not title?: string
isDisabled?: boolean,
icon?: React.ComponentType<any> | React.Element<any>,
navButtonProps?: Object,
};Since title is required, Flow will catch any missing titles at compile time, making the runtime fallback unnecessary. The code can be simplified:
Line 239 (Box AI custom tab):
- tooltip={boxAiTab.title ?? boxAiTab.id}
+ tooltip={boxAiTab.title}Line 293 (other custom tabs):
- tooltip={customTabTitle ?? customTabId}
+ tooltip={customTabTitle}Both fallbacks should be removed since the type system guarantees title will always be present.
🧬 Code Graph Analysis Results
[src/elements/content-sidebar/SidebarNav.js] // renderCustomPanelIcon and related fallback logic for tab tooltips and icons
/*
Snippet 1: Fallback tooltip for Box AI custom tab
Context: When a custom or Box AI tab is rendered, the tooltip uses the tab title if provided, otherwise it falls back to the tab id.
Relevant lines:
*/
boxAiTab && (
<SidebarNavButton
key={boxAiTab.id}
isPreviewModernizationEnabled={isPreviewModernizationEnabled}
data-target-id={SidebarNavButton-${boxAiTab.id}}
data-testid={sidebar${boxAiTab.id}}
{...boxAiTab.navButtonProps}
data-resin-target={SIDEBAR_NAV_TARGETS.BOXAI}
isDisabled={boxAiTab.isDisabled}
onClick={handleSidebarNavButtonClick}
sidebarView={boxAiTab.path}
tooltip={boxAiTab.title ?? boxAiTab.id}
>
{renderCustomPanelIcon(
boxAiTab.icon,
isPreviewModernizationEnabled,
<BoxAiLogo24 {...SIDEBAR_TAB_ICON_PROPS} />,
,
)}
),
/*
Snippet 2: Fallback tooltip for additional/custom tabs
Context: For any non-BoxAI custom tab, the tooltip falls back to title if provided, otherwise the id.
Relevant lines:
*/
!hasNativeBoxAISidebar && boxAiTab && (
<SidebarNavButton
key={boxAiTab.id}
isPreviewModernizationEnabled={isPreviewModernizationEnabled}
data-target-id={SidebarNavButton-${boxAiTab.id}}
data-testid={sidebar${boxAiTab.id}}
{...boxAiTab.navButtonProps}
data-resin-target={SIDEBAR_NAV_TARGETS.BOXAI}
isDisabled={boxAiTab.isDisabled}
onClick={handleSidebarNavButtonClick}
sidebarView={boxAiTab.path}
tooltip={boxAiTab.title ?? boxAiTab.id}
>
{renderCustomPanelIcon(
boxAiTab.icon,
isPreviewModernizationEnabled,
<BoxAiLogo24 {...SIDEBAR_TAB_ICON_PROPS} />,
,
)}
),
/*
Snippet 3: Fallback tooltip for user-provided custom tabs (title or id)
Context: For each custom tab in customTabs (excluding Box AI), tooltip uses title if provided, otherwise id.
Relevant lines:
*/
customTabs &&
customTabs.filter(tab => tab.id !== SIDEBAR_VIEW_BOXAI).map(customTab => {
const {
id: customTabId,
path: customTabPath,
icon: CustomTabIcon,
title: customTabTitle,
navButtonProps,
} = customTab;
const customTabButton = (
<SidebarNavButton
key={customTabId}
isPreviewModernizationEnabled={isPreviewModernizationEnabled}
data-resin-target={`sidebar${customTabId}`}
data-target-id={`SidebarNavButton-${customTabId}`}
data-testid={`sidebar${customTabId}`}
{...navButtonProps}
isDisabled={customTab.isDisabled}
onClick={handleSidebarNavButtonClick}
sidebarView={customTabPath}
tooltip={customTabTitle ?? customTabId}
>
{renderCustomPanelIcon(
CustomTabIcon,
isPreviewModernizationEnabled,
<InformationCircleIcon {...SIDEBAR_TAB_ICON_PROPS} />,
<IconDocInfo className="bcs-SidebarNav-icon" />,
)}
</SidebarNavButton>
);
visibleTabs.push(customTabButton); // Add at the end
}),
/*
Snippet 4: Icon rendering helper with support for both React elements and component types
Context: renderCustomPanelIcon handles:
-
no icon provided -> return default modern/legacy icons based on feature flag
-
React element -> return as-is
-
Component type -> render with appropriate props
*/
const renderCustomPanelIcon = (
icon?: React.ComponentType | React.Element,
isPreviewModernizationEnabled: boolean,
defaultModernIcon: React.Node,
defaultLegacyIcon: React.Node,
): React.Node => {
if (!icon) {
return isPreviewModernizationEnabled ? defaultModernIcon : defaultLegacyIcon;
}if (React.isValidElement(icon)) {
// Icon is already a React element, return as-is
return (icon: any);
}// Icon is a component type, render it
const IconComponent: React.ComponentType = (icon: any);
return ;
};
🧠 Learnings used
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.
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.
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4228
File: src/elements/content-sidebar/activity-feed/comment/Comment.js:147-148
Timestamp: 2025-08-19T17:50:47.187Z
Learning: In the box-ui-elements codebase, ActivityThreadReplyForm.js and AnnotationThreadCreate.js component files exist but are not actively used/imported anywhere, so they don't need updates for prop propagation.
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.
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.
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarPanels.js:0-0
Timestamp: 2025-09-03T18:24:37.905Z
Learning: User fpan225 prefers to keep the original `customPanels` variable name rather than normalizing it to a different variable name (like `panels`) for better code readability. They use `if (hasCustomPanels && customPanels)` pattern to satisfy Flow type checking instead of creating a normalized array.
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarPanels.js:0-0
Timestamp: 2025-09-23T21:14:20.232Z
Learning: User fpan225 clarified that Box AI is included in customPanels if it exists, but the current getPanelOrder function logic in the else branch still excludes Box AI from the returned order when shouldBoxAIBeDefaultPanel is false, since otherCustomPanelPaths specifically filters out Box AI panels.
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.
There was a problem hiding this comment.
@fpan225 Please consider making the updates ^ and double checking if we do that anywhere else
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| <SidebarNavButton | ||
| key={boxAiTab.id} | ||
| isPreviewModernizationEnabled={isPreviewModernizationEnabled} | ||
| data-target-id={`SidebarNavButton-$boxAI`} |
There was a problem hiding this comment.
Should the $ be here? using https://github.com/box/box-ui-elements/pull/4326/changes#diff-97eebc87a1ef0cb32fc6b625599429bd1fa4171939388b3fe939e6e3227bfc15R212 as reference
Summary by CodeRabbit
New Features
UI / Behavior
Tests
Documentation / Stories
✏️ Tip: You can customize this high-level summary in your review settings.