feat(content-sidebar): implement for custom sidebar panels#4326
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 a new exported Flow type Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant ContentSidebar as ContentSidebar
participant Sidebar as Sidebar
participant SidebarNav as SidebarNav
participant SidebarPanels as SidebarPanels
User->>ContentSidebar: mount(props { customSidebarPanels? })
ContentSidebar->>Sidebar: render(customSidebarPanels)
Sidebar->>SidebarNav: render(customSidebarPanels, hasNativeBoxAISidebar)
Sidebar->>SidebarPanels: render(customSidebarPanels, hasNativeBoxAISidebar)
SidebarNav->>SidebarNav: compute visibleTabs(hasNativeBoxAISidebar, customSidebarPanels)
SidebarPanels->>SidebarPanels: compute panelOrder(defaults, customSidebarPanels, hasNativeBoxAISidebar)
User->>SidebarNav: click tab (panelId)
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 | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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.
| <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
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 339-348: The empty-panels guard currently checks
hasNativeBoxAISidebar which remains true when showOnlyBoxAINavButton is set,
causing a false-positive "has panel" condition; update the guard to use
canShowBoxAISidebarPanel instead so the logic respects the computed visibility
(replace hasNativeBoxAISidebar with canShowBoxAISidebarPanel in the conditional
that decides whether to short-circuit when no panels are available, e.g., in the
block containing isOpen and the list of
hasActivity/hasDetails/.../hasCustomPanels).
♻️ Duplicate comments (1)
src/elements/content-sidebar/SidebarNav.js (1)
230-238: Fix Box AI custom tab data attributes and override order.
data-target-idis hardcoded andnavButtonPropsis spread beforedata-resin-target, preventing consumers from overriding analytics attributes.✅ Proposed fix
- data-target-id={`SidebarNavButton-boxAI`} - data-testid={`sidebar${boxAiPanel.id}`} - {...boxAiPanel.navButtonProps} - data-resin-target={SIDEBAR_NAV_TARGETS.BOXAI} + data-resin-target={SIDEBAR_NAV_TARGETS.BOXAI} + data-target-id={`SidebarNavButton-${boxAiPanel.id}`} + data-testid={`sidebar${boxAiPanel.id}`} + {...boxAiPanel.navButtonProps}Based on learnings, keep
navButtonPropsafter explicitdata-*props to allow overrides.
🧹 Nitpick comments (1)
src/elements/content-sidebar/SidebarPanels.js (1)
395-397: Drop redundantCustomPanelComponentnull check.
componentis required inCustomSidebarPanel, so this check can be removed.Based on learnings, rely on the Flow type contract instead of runtime checks.
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)
315-349: Treat all‑disabled custom panels as “no panels” in the guard.
hasCustomPanelsis length‑based, so if all custom panels are disabled and no native panels are eligible, the guard won’t short‑circuit and the fallback redirect can loop to/with no eligible panel. Consider tracking eligibility and using that in the guard.💡 Suggested adjustment
- const customPanelEligibility = {}; + const customPanelEligibility = {}; + let hasEligibleCustomPanels = false; if (hasCustomPanels) { // $FlowFixMe: customSidebarPanels is checked for existence in hasCustomPanels customSidebarPanels.forEach(({ id, path, isDisabled }) => { const isBoxAICustomPanel = id === SIDEBAR_VIEW_BOXAI; const isEligible = isBoxAICustomPanel ? !canShowBoxAISidebarPanel && !isDisabled : !isDisabled; customPanelEligibility[path] = isEligible; + if (isEligible) { + hasEligibleCustomPanels = true; + } }); } ... - !hasVersions && - !hasCustomPanels) + !hasVersions && + !hasEligibleCustomPanels) ) { return null; }
♻️ Duplicate comments (1)
src/elements/content-sidebar/__tests__/SidebarPanels.test.js (1)
814-822: Use DOM testids instead of the non‑existentCustomPanelselector.The test is asserting against a component that doesn’t exist in the render output, so it isn’t actually validating the DOM.
✅ 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: 1
🤖 Fix all issues with AI agents
In `@src/elements/content-sidebar/__tests__/SidebarPanels.test.js`:
- Around line 838-842: The test uses invalid component selectors
'CustomPanel[id="custom1"]' and 'CustomPanel[id="custom2"]' which don't
correspond to rendered elements; update the assertions to check the actual
rendered testids instead (e.g., replace
wrapper.exists('CustomPanel[id="custom1"]') /
wrapper.exists('CustomPanel[id="custom2"]') with
wrapper.exists('[data-testid="custom1"]') /
wrapper.exists('[data-testid="custom2"]') or the project's equivalent testid
selectors) so the assertions target the real DOM nodes alongside the existing
wrapper.exists('BoxAISidebar') check.
fpan225
force-pushed
the
custom-sidebar-panels
branch
from
c3e35b5 to
08cdea1
Compare
| test('should render custom panels with regular panels', () => { | ||
| const customSidebarPanels = [ | ||
| createCustomPanel('analytics', { title: 'Analytics Panel' }), | ||
| createCustomPanel('reports', { title: 'Reports Panel' }), |
There was a problem hiding this comment.
we should assert for reports also
| expect(disabledWrapper.find('div[data-testid="disabled-sidebar"]')).toHaveLength(0); | ||
| }); | ||
|
|
||
| test('should render custom panels without Box AI', () => { |
There was a problem hiding this comment.
| */ | ||
| const renderCustomPanelIcon = ( | ||
| icon?: React.ComponentType<any> | React.Element<any>, | ||
| isPreviewModernizationEnabled: boolean, |
There was a problem hiding this comment.
should this flag be part of this component? customPanelIcon should be generic
| if (hasNativeBoxAISidebar) { | ||
| return shouldBoxAIBeDefaultPanel | ||
| ? [SIDEBAR_VIEW_BOXAI, ...DEFAULT_SIDEBAR_VIEWS, ...nonBoxAIPaths] | ||
| : [...DEFAULT_SIDEBAR_VIEWS, SIDEBAR_VIEW_BOXAI, ...nonBoxAIPaths]; | ||
| } | ||
|
|
||
| if (boxAiCustomPanel && shouldBoxAIBeDefaultPanel) { | ||
| return [boxAiCustomPanel.path, ...DEFAULT_SIDEBAR_VIEWS, ...nonBoxAIPaths]; | ||
| } |
There was a problem hiding this comment.
do we think Box AI sidebar may be used without being first? I feel like we could just assume that Box AI is always first and then we don't need shouldBoxAIBeDefaultPanel and additional logic
There was a problem hiding this comment.
Yes, Box AI is always first according to product requirement in EUA, will remove shouldBoxAIBeDefaultPanel here
| const { | ||
| activitySidebarProps, | ||
| boxAISidebarProps, | ||
| customSidebarPanels, |
There was a problem hiding this comment.
default this to an empty array so you don't need to keep checking for existence in the following conditionals
| customSidebarPanels, | |
| customSidebarPanels = [], |
There was a problem hiding this comment.
i.e. remove all the conditions in this file that check for the existence of customSidebarPanels
| customSidebarPanels?: Array<CustomSidebarPanel>, | ||
| shouldBoxAIBeDefaultPanel: boolean, | ||
| hasNativeBoxAISidebar: boolean, |
There was a problem hiding this comment.
it's weird that the first argument is optional but then the next arguments are required. if you set a default value for customSidebarPanels when it's taken from props, then you don't need to have it optional here
| const boxAiCustomPanel = customSidebarPanels | ||
| ? customSidebarPanels.find(panel => panel.id === SIDEBAR_VIEW_BOXAI) | ||
| : undefined; | ||
| const boxAiPath = boxAiCustomPanel ? boxAiCustomPanel.path : null; | ||
| const nonBoxAIPaths = customPanelPaths.filter(path => path !== boxAiPath); | ||
|
|
||
| if (hasNativeBoxAISidebar) { | ||
| return shouldBoxAIBeDefaultPanel | ||
| ? [SIDEBAR_VIEW_BOXAI, ...DEFAULT_SIDEBAR_VIEWS, ...nonBoxAIPaths] | ||
| : [...DEFAULT_SIDEBAR_VIEWS, SIDEBAR_VIEW_BOXAI, ...nonBoxAIPaths]; | ||
| } | ||
|
|
||
| if (boxAiCustomPanel && shouldBoxAIBeDefaultPanel) { | ||
| return [boxAiCustomPanel.path, ...DEFAULT_SIDEBAR_VIEWS, ...nonBoxAIPaths]; | ||
| } |
There was a problem hiding this comment.
isn't boxAiCustomPanel.path the same value as SIDEBAR_VIEW_BOXAI? if so, then you shouldn't need to separate out the logic
There was a problem hiding this comment.
this function could be written as:
getPanelOrder = (customPanels, hasNativeBoxAiPanel, isBoxAiDefaultPanel) => {
const hasCustomBoxAiPanel = customPanels.some(({ id }) => id === SIDEBAR_VIEW_BOXAI);
if ((hasNativeBoxAiPanel || hasCustomBoxAiPanel) && isBoxAiDefaultPanel) {
const customPaths = customPanels.map(({ path }) => path);
const otherPaths = customPaths.filter(path => path !== SIDEBAR_VIEW_BOXAI);
return [SIDEBAR_VIEW_BOXAI, ...DEFAULT_SIDEBAR_VIEWS, ...otherPaths];
}
return [...DEFAULT_SIDEBAR_VIEWS, ...customPanelPaths];
};
| // $FlowFixMe: customSidebarPanels is checked for existence in hasCustomPanels | ||
| customSidebarPanels.forEach(({ id, path, isDisabled }) => { | ||
| const isBoxAICustomPanel = id === SIDEBAR_VIEW_BOXAI; | ||
| const isEligible = isBoxAICustomPanel ? !canShowBoxAISidebarPanel && !isDisabled : !isDisabled; |
There was a problem hiding this comment.
this ternary is pretty confusing to read. maybe something like:
customSidebarPanels.forEach(({ id, isDisabled, path }) => {
if (id === SIDEBAR_VIEW_BOXAI && canShowBoxAISidebarPanel) {
return;
}
customPanelsEligibility[path] = !isDisabled;
});
There was a problem hiding this comment.
VRTs are expensive, do we need all of these?
There was a problem hiding this comment.
Good point, will remove redundant ones
| }; | ||
| }; | ||
|
|
||
| const getWrapper = ({ path = '/', customSidebarPanels, ...rest } = {}) => { |
There was a problem hiding this comment.
you don't need to add customSidebarPanels, it'll be spread with the rest
| <MemoryRouter initialEntries={[path]}> | ||
| <SidebarPanels | ||
| file={{ id: '1234' }} | ||
| customSidebarPanels={customSidebarPanels} |
There was a problem hiding this comment.
you don't need to add customSidebarPanels, it'll be spread with rest
| ${'/'} | ${'DocGenSidebar'} | ||
| `('should render $sidebar given the path $path', ({ path, sidebar }) => { | ||
| const wrapper = getWrapper({ path }); | ||
| const customSidebarPanels = sidebar === 'BoxAISidebar' ? [createBoxAIPanel()] : undefined; |
There was a problem hiding this comment.
you're removing the tests for native box ai sidebar. you should be adding a separate test for custom sidebar and leave these as-is
There was a problem hiding this comment.
same comment for the other tests changed in this file
| }); | ||
|
|
||
| test('should call focusBoxAISidebarPrompt when clicked on Box AI Tab', async () => { | ||
| test('should call focusBoxAISidebarPrompt when clicked on custom Box AI Tab', async () => { |
There was a problem hiding this comment.
I don't think you should be changing the tests in this file to from native to custom. same comment for all the other tests
because then you're testing specific behavior for a "custom" panel. shouldn't the logic and test for focusPrompt now live inside box-ai-client or EUA?
There was a problem hiding this comment.
Agree this is redundant, will remove
…nhance type definitions
…idebar and SidebarPanels
…ation and custom panels
…nce SidebarPanels logic
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 (1)
src/elements/content-sidebar/SidebarPanels.js (1)
306-335:⚠️ Potential issue | 🟡 MinorAvoid treating all-disabled custom panels as “available.”
If all custom panels are disabled and no native panels are available, the guard won’t short-circuit and you’ll redirect to
/with no eligible route. Consider checking for eligible custom panels.🔧 Proposed fix
- const hasCustomPanels = customSidebarPanels.length > 0; + const hasCustomPanels = customSidebarPanels.length > 0; + const hasEligibleCustomPanels = customSidebarPanels.some(panel => !panel.isDisabled); @@ - !hasDocGen && - !hasVersions && - !hasCustomPanels) + !hasDocGen && + !hasVersions && + !hasEligibleCustomPanels)
🤖 Fix all issues with AI agents
In `@src/elements/content-sidebar/SidebarPanels.js`:
- Around line 249-263: getPanelOrder currently ignores shouldBoxAIBeDefaultPanel
and always forces Box AI to the front when a native or custom Box AI exists;
update getPanelOrder to respect shouldBoxAIBeDefaultPanel by computing a flag
(e.g., includeBoxAI = hasNativeBoxAISidebar || (hasCustomBoxAIPanel &&
shouldBoxAIBeDefaultPanel)) and use that to decide ordering: when includeBoxAI
is true return [SIDEBAR_VIEW_BOXAI, ...DEFAULT_SIDEBAR_VIEWS, ...nonBoxAIPaths],
otherwise return [...DEFAULT_SIDEBAR_VIEWS, ...customPanelPaths] so custom
Sidebar panels (including Box AI) are preserved in their original positions when
shouldBoxAIBeDefaultPanel is false.
🧹 Nitpick comments (1)
src/elements/content-sidebar/SidebarPanels.js (1)
366-377: Drop the redundant component existence check.The type already requires
component, so the runtime check just adds noise.Based on learnings, In the CustomSidebarPanel type, the component field is required (React.ComponentType), 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.♻️ Proposed cleanup
- if (isDisabled || !CustomPanelComponent) { + if (isDisabled) { return null; }
| getPanelOrder = ( | ||
| customSidebarPanels: Array<CustomSidebarPanel> = [], | ||
| shouldBoxAIBeDefaultPanel: boolean, | ||
| hasNativeBoxAISidebar: boolean, | ||
| ): string[] => { | ||
| const customPanelPaths = customSidebarPanels.map(panel => panel.path); | ||
| const hasCustomBoxAIPanel = customSidebarPanels.some(({ id }) => id === SIDEBAR_VIEW_BOXAI); | ||
| const nonBoxAIPaths = customPanelPaths.filter(path => path !== SIDEBAR_VIEW_BOXAI); | ||
|
|
||
| if (hasNativeBoxAISidebar || hasCustomBoxAIPanel) { | ||
| return [SIDEBAR_VIEW_BOXAI, ...DEFAULT_SIDEBAR_VIEWS, ...nonBoxAIPaths]; | ||
| } | ||
|
|
||
| return [...DEFAULT_SIDEBAR_VIEWS, ...customPanelPaths]; | ||
| }; |
There was a problem hiding this comment.
Honor shouldBoxAIBeDefaultPanel in panel ordering.
getPanelOrder currently ignores the feature flag and always puts Box AI first when it exists, which can change the default redirect behavior.
✅ Proposed fix
- if (hasNativeBoxAISidebar || hasCustomBoxAIPanel) {
- return [SIDEBAR_VIEW_BOXAI, ...DEFAULT_SIDEBAR_VIEWS, ...nonBoxAIPaths];
- }
-
- return [...DEFAULT_SIDEBAR_VIEWS, ...customPanelPaths];
+ const hasBoxAIPanel = hasNativeBoxAISidebar || hasCustomBoxAIPanel;
+ if (!hasBoxAIPanel) {
+ return [...DEFAULT_SIDEBAR_VIEWS, ...customPanelPaths];
+ }
+ if (shouldBoxAIBeDefaultPanel) {
+ return [SIDEBAR_VIEW_BOXAI, ...DEFAULT_SIDEBAR_VIEWS, ...nonBoxAIPaths];
+ }
+ return [...DEFAULT_SIDEBAR_VIEWS, SIDEBAR_VIEW_BOXAI, ...nonBoxAIPaths];🧰 Tools
🪛 Biome (2.3.13)
[error] 250-250: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 251-251: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 252-252: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 253-253: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
🤖 Prompt for AI Agents
In `@src/elements/content-sidebar/SidebarPanels.js` around lines 249 - 263,
getPanelOrder currently ignores shouldBoxAIBeDefaultPanel and always forces Box
AI to the front when a native or custom Box AI exists; update getPanelOrder to
respect shouldBoxAIBeDefaultPanel by computing a flag (e.g., includeBoxAI =
hasNativeBoxAISidebar || (hasCustomBoxAIPanel && shouldBoxAIBeDefaultPanel)) and
use that to decide ordering: when includeBoxAI is true return
[SIDEBAR_VIEW_BOXAI, ...DEFAULT_SIDEBAR_VIEWS, ...nonBoxAIPaths], otherwise
return [...DEFAULT_SIDEBAR_VIEWS, ...customPanelPaths] so custom Sidebar panels
(including Box AI) are preserved in their original positions when
shouldBoxAIBeDefaultPanel is false.
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/__tests__/SidebarPanels.test.js (1)
25-68:⚠️ Potential issue | 🔴 CriticalDefine
hasNativeBoxAISidebarbefore shorthand JSX use.
hasNativeBoxAISidebaris referenced as a shorthand prop ingetWrapper/getSidebarPanelsbut not declared in scope. When these helpers are called with onlypath(e.g., line 93:getWrapper({ path })), this will throw a ReferenceError at render time.🔧 Suggested fix
- const getWrapper = ({ path = '/', ...rest } = {}) => { + const getWrapper = ({ path = '/', hasNativeBoxAISidebar = true, ...rest } = {}) => { return mount( <SidebarPanels customSidebarPanels={[]} file={{ id: '1234' }} hasDocGen hasActivity hasDetails hasMetadata hasNativeBoxAISidebar hasSkills hasVersions isOpen {...rest} />, { @@ - const getSidebarPanels = ({ path = '/', ...props }) => { + const getSidebarPanels = ({ path = '/', hasNativeBoxAISidebar = true, ...props }) => { return ( <MemoryRouter initialEntries={[path]}> <SidebarPanels customSidebarPanels={[]} file={{ id: '1234' }} hasDocGen hasActivity hasDetails hasMetadata hasNativeBoxAISidebar hasSkills hasVersions isOpen {...props} /> </MemoryRouter> ); };
| const canShowBoxAISidebarPanel = hasBoxAI && !showOnlyBoxAINavButton; | ||
| const canShowBoxAISidebarPanel = hasNativeBoxAISidebar && !showOnlyBoxAINavButton; | ||
|
|
||
| const hasCustomPanels = customSidebarPanels.length > 0; |
There was a problem hiding this comment.
update line 269 to default customSidebarPanels = [] to avoid null.length or undefined.length case which will cause error.
There was a problem hiding this comment.
The case where customSidebarPanels is null or undefined by defaulting to an empty array already handled during prop destructuring in the render() method in
7 participants
Summary
New Features
UI / Behavior
Tests
Documentation / Stories
snapshot
Summary by CodeRabbit
New Features
Tests