feat(content-sidebar): implement for custom sidebar panels#4326
Merged
mergify[bot] merged 23 commits intomasterfrom Feb 6, 2026
Merged
feat(content-sidebar): implement for custom sidebar panels#4326mergify[bot] merged 23 commits intomasterfrom
mergify[bot] merged 23 commits intomasterfrom
Conversation
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Contributor
WalkthroughAdds a new 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 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
🧪 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 |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
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; + );
Contributor
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);
Contributor
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)} + /> + ); }} /> );
Contributor
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.
jpan-box
reviewed
Jan 22, 2026
jpan-box
reviewed
Jan 22, 2026
jpan-box
reviewed
Jan 22, 2026
Contributor
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.
jpan-box
previously approved these changes
Jan 22, 2026
Contributor
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);
Contributor
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.
c3e35b5 to
08cdea1
Compare
greg-in-a-box
requested changes
Jan 29, 2026
tjuanitas
reviewed
Jan 29, 2026
src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx
Show resolved
Hide resolved
…nhance type definitions
…idebar and SidebarPanels
…ation and custom panels
…nce SidebarPanels logic
fpan225
force-pushed
the
custom-sidebar-panels
branch
from
e11a2b7 to
ed1dae4
Compare
kduncanhsu
approved these changes
Feb 5, 2026
greg-in-a-box
approved these changes
Feb 6, 2026
Contributor
Merge Queue Status✅ The pull request has been merged at 5a67dd5 This pull request spent 7 seconds in the queue, with no time running CI. Required conditions to merge
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
New Features
UI / Behavior
Tests
Documentation / Stories
snapshot
Summary by CodeRabbit
New Features
Tests