feat(metadata-view): Implement metadata sidepanel #4230
Conversation
WalkthroughAdds a feature-flagged Metadata Side Panel to Content Explorer, threads selection state and a panel toggle through SubHeader components, extracts selection-label utilities, updates layout/styles, and adds tests plus a Storybook visual test. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SubHeaderRight
participant SubHeader
participant ContentExplorer
participant MetadataSidePanel
User->>SubHeaderRight: Click "Metadata" button
SubHeaderRight->>SubHeader: call onMetadataSidePanelToggle()
SubHeader->>ContentExplorer: invoke onMetadataSidePanelToggle()
ContentExplorer->>ContentExplorer: toggle isMetadataSidePanelOpen
alt feature enabled & metadata view & open
ContentExplorer-->>MetadataSidePanel: render with selectedItemIds
end
sequenceDiagram
participant MetadataView
participant ContentExplorer
participant MetadataSidePanel
MetadataView->>ContentExplorer: onSelectionChange(ids)
ContentExplorer->>ContentExplorer: set selectedItemIds
alt selection empty
ContentExplorer->>ContentExplorer: closeMetadataSidePanel()
ContentExplorer--x MetadataSidePanel: Unmount
else selection non-empty
ContentExplorer->>MetadataSidePanel: pass selectedItemIds
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
src/elements/content-explorer/ContentExplorer.scss (1)
24-26: Use logical margin for RTL support (or prefer flex gap on the container).Using margin-left will render incorrectly in RTL locales. Either switch to margin-inline-start or move spacing responsibility to the flex container via column-gap.
Option A: minimally fix RTL
- margin-left: var(--space-4); + margin-inline-start: var(--space-4);Option B: prefer container gap and remove panel margin (cleaner as layout concern belongs to the flex container)
.bce-container { display: flex; height: 100%; min-height: 0; + column-gap: var(--space-4); }.bce-sidepanel { - margin-left: var(--space-4); }src/elements/content-explorer/MetadataSidePanel.scss (1)
19-21: Avoid hiding UI via a brittle data-target selector; prefer a prop-driven or semantic control.Targeting [data-target-id='TextButton-deleteButton'] in CSS is fragile and risks regressions if the attribute or component structure changes. It also couples styling to an implementation/testing detail.
Recommended approaches:
- Hide the delete action at the component level (e.g., pass a prop to omit it from render or conditionally filter actions).
- If style-only is required, use a more stable className on the specific button provided by the component API.
- As a last resort, add a BEM/namespace wrapper for the specific panel section and scope this rule tightly to avoid unintended effects.
I can help wire a prop through the component tree to omit the delete button entirely — want me to draft that?
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
136-160: Make the play-step more robust by awaiting rendered elements before interaction.Minimize flakiness by using findByRole instead of a manual waitFor + getByRole and by awaiting the Metadata button before clicking.
Apply this diff:
play: async ({ canvas }) => { - await waitFor(() => { - expect(canvas.getByRole('row', { name: /Child 2/i })).toBeInTheDocument(); - }); - - // Select the first row by clicking its checkbox - const firstRow = canvas.getByRole('row', { name: /Child 2/i }); - const checkbox = within(firstRow).getByRole('checkbox'); + const firstRow = await canvas.findByRole('row', { name: /Child 2/i }); + const checkbox = within(firstRow).getByRole('checkbox'); // within(...) after findByRole avoids race await userEvent.click(checkbox); - const metadataButton = canvas.getByRole('button', { name: 'Metadata' }); + const metadataButton = await canvas.findByRole('button', { name: 'Metadata' }); await userEvent.click(metadataButton); },Optional: If possible, assert the panel opened by awaiting a stable element unique to the panel (e.g., its header/title or a landmark/region role).
src/elements/common/sub-header/SubHeader.tsx (1)
31-31: Consider documenting the toggle callback's behavior.The optional
onToggleMetadataSidePanelprop should have a JSDoc comment explaining when it's called and what UI state it affects.Add documentation above the interface property:
onSortChange: (sortBy: string, sortDirection: string) => void; + /** Callback triggered when the metadata side panel toggle button is clicked */ onToggleMetadataSidePanel?: () => void;src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (1)
65-127: Consider adding test coverage for edge cases.While the test suite covers the main functionality well, consider adding tests for:
- Empty collection handling
- Error states when metadata template is missing or invalid
- Keyboard navigation/accessibility
- Multiple template scenarios
Would you like me to generate additional test cases for these edge cases?
src/elements/content-explorer/utils.ts (2)
51-52: Document the TODO for multiple item support.The TODO comment indicates incomplete functionality for multiple selected items. This should be tracked properly.
Would you like me to open an issue to track the implementation of multiple selected items support? This would involve aggregating field values across all selected items and handling conflicts appropriately.
32-66: Consider adding error boundary for metadata template processing.The
getTemplateInstancefunction should handle cases where the metadata template or selected items have unexpected structures.Consider wrapping the field mapping in a try-catch or adding validation:
export function getTemplateInstance(metadataTemplate: MetadataTemplate, selectedItems: BoxItem[]) { const { displayName, fields, hidden, id, scope, templateKey, type } = metadataTemplate; + + if (!selectedItems.length) { + return { + canEdit: true, + displayName, + hidden, + id, + fields: [], + scope, + templateKey, + type, + }; + }src/elements/content-explorer/ContentExplorer.tsx (1)
1856-1867: Consider extracting the side panel visibility condition.The condition for showing the metadata side panel is complex and could be extracted for better readability.
+ const shouldShowMetadataSidePanel = + isDefaultViewMetadata && + isMetadataViewV2Feature && + this.state.isMetadataSidePanelOpen; + // ... in render - {isDefaultViewMetadata && - isMetadataViewV2Feature && - this.state.isMetadataSidePanelOpen && ( + {shouldShowMetadataSidePanel && ( <div className="bce-sidepanel">src/elements/content-explorer/MetadataSidePanel.tsx (3)
79-83: Hide decorative icon from assistive techThe file icon is purely decorative next to the subtitle. Mark it as hidden to reduce noise for screen readers.
Apply this diff:
- <div className="bce-sidepanel-subtitle"> - <FileDefault /> + <div className="bce-sidepanel-subtitle"> + <FileDefault aria-hidden="true" /> <Text as="span" color="textOnLightSecondary" variant="subtitle"> {selectedItemText} </Text> </div>
96-96: Avoid passing null to optional function propsPassing
nullto props that are likely treated as optional functions/fetchers can cause “null is not a function” at runtime. Prefer omitting the prop or usingundefined.The previous diff already removes
fetchSuggestions={null}and replaces other= {null}with= {undefined}. Please confirm the typings in@box/metadata-editorand@box/blueprint-webaccept omitted/undefined values. If they require explicit no-op callbacks, we can wire safe stubs instead.
41-46: Minor: memoize selectedItems to avoid unnecessary recomputationNot critical, but
filterruns on every render. You can memoizeselectedItemsbased oncurrentCollection.itemsandselectedItemIds.Example:
-import React, { useState } from 'react'; +import React, { useMemo, useState } from 'react'; @@ - const selectedItems = - selectedItemIds === 'all' - ? currentCollection.items - : currentCollection.items.filter(item => selectedItemIds.has(item.id)); + const selectedItems = useMemo( + () => + selectedItemIds === 'all' + ? currentCollection.items + : currentCollection.items.filter(item => selectedItemIds.has(item.id)), + [currentCollection.items, selectedItemIds], + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
src/elements/common/sub-header/SubHeader.tsx(3 hunks)src/elements/common/sub-header/SubHeaderLeftV2.tsx(2 hunks)src/elements/common/sub-header/SubHeaderRight.tsx(5 hunks)src/elements/content-explorer/ContentExplorer.scss(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(7 hunks)src/elements/content-explorer/MetadataSidePanel.scss(1 hunks)src/elements/content-explorer/MetadataSidePanel.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(3 hunks)src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx(1 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)src/elements/content-explorer/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/elements/common/sub-header/SubHeaderLeftV2.tsx (1)
src/elements/content-explorer/utils.ts (1)
useSelectedItemText(11-29)
src/elements/content-explorer/MetadataSidePanel.tsx (1)
src/elements/content-explorer/utils.ts (2)
useSelectedItemText(11-29)getTemplateInstance(32-66)
src/elements/content-explorer/ContentExplorer.tsx (2)
src/elements/content-picker/Content.js (1)
Content(51-98)src/features/metadata-based-view/__tests__/MetadataBasedItemList.test.js (1)
fieldsToShow(102-107)
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (3)
src/elements/content-explorer/ContentExplorer.tsx (1)
ContentExplorer(1969-1969)src/elements/common/__mocks__/mockMetadata.ts (1)
mockSchema(226-226)src/features/metadata-based-view/__tests__/MetadataBasedItemList.test.js (1)
fieldsToShow(102-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (15)
src/elements/content-explorer/MetadataSidePanel.scss (1)
1-18: Solid structural styles for the panel; flex alignment and spacing look good.The basic layout (header/title/subtitle/content) and use of spacing tokens are consistent and clear. No issues from a layout perspective.
src/elements/common/sub-header/SubHeaderLeftV2.tsx (1)
24-24: Good reuse of shared hook for selection text.Shifting to useSelectedItemText centralizes logic and keeps SubHeaderLeftV2 lean. Dependencies and memoization in the hook look appropriate given immutable updates to items.
src/elements/common/sub-header/SubHeaderRight.tsx (1)
100-104: Conditional rendering of the Metadata button is clean and correctly gated.Gating on view, feature flag, and selection state avoids accidental surfacing of the action. Wiring onClick to onToggleMetadataSidePanel keeps the component stateless.
src/elements/common/sub-header/SubHeader.tsx (1)
114-114: Props are correctly forwarded to SubHeaderRight.The implementation properly passes the new
onToggleMetadataSidePanelandselectedItemIdsprops to the SubHeaderRight component for handling the metadata button visibility and actions.Also applies to: 118-118
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (1)
7-10: Good practice mocking the scrollTo method.Mocking
Element.prototype.scrollToprevents test failures when the component attempts to scroll, which is appropriate for unit tests.src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (2)
426-487: Well-structured metadata view V2 test setup.The test properly sets up the metadata view V2 configuration with dynamic columns, fields, and query construction based on the mock schema. The implementation correctly enables the feature flag and validates the UI behavior.
489-504: Comprehensive UI interaction test.The test effectively validates the metadata view button visibility after selection and proper rendering of the content explorer with metadata view V2 enabled.
src/elements/content-explorer/utils.ts (1)
11-29: Efficient selection text computation with proper memoization.The
useSelectedItemTexthook correctly handles all selection states (empty, single, multiple, and 'all') with appropriate internationalization and memoization.src/elements/content-explorer/ContentExplorer.tsx (6)
173-173: State property added correctly for metadata side panel.The
isMetadataSidePanelOpenboolean state is properly initialized tofalseand follows the existing state management pattern.Also applies to: 299-299
1549-1554: Good integration with selection change handler.The code correctly closes the metadata side panel when selection becomes empty, ensuring consistent UI state.
1634-1639: Consistent panel closing on selection clear.The
clearSelectedItemIdsmethod properly closes the metadata side panel when clearing selections, maintaining UI consistency.
1647-1651: Well-implemented toggle and close methods.The toggle and close methods for the metadata side panel are simple and follow React best practices.
Also applies to: 1659-1661
1779-1868: Clean layout implementation with proper feature gating.The render method correctly:
- Wraps content in a container/main structure for the two-column layout
- Feature-gates the side panel rendering
- Passes all required props to the MetadataSidePanel component
1860-1865: MetadataSidePanelProps VerifiedThe props passed to
MetadataSidePanel(currentCollection,closeMetadataSidePanel,metadataTemplate,selectedItemIds) exactly match its exportedMetadataSidePanelPropsinterface. No further changes needed.src/elements/content-explorer/MetadataSidePanel.tsx (1)
31-40: Solid initial structure and state wiringProps are clearly defined; view/edit toggling and unsaved changes flow are set up for future save logic. Good separation of edit vs view modes.
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx
Outdated
Show resolved
Hide resolved
tjiang-box
force-pushed
the
metadata-sidepanel
branch
from
c04c623 to
7a1372d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/elements/content-explorer/ContentExplorer.tsx (7)
451-455: Ensure V2 helper is structurally compatible with the original helper
this.metadataQueryAPIHelperis declared asMetadataQueryAPIHelper, but can receive an instance ofMetadataQueryAPIHelperV2. This relies on structural compatibility. Verify both classes expose the same surface used here:fetchMetadataQueryResultsandupdateMetadatawith compatible signatures. If not, typing should be adjusted to a shared interface.Optionally define a minimal interface (e.g.,
IMetadataQueryHelper) capturing the used methods and typemetadataQueryAPIHelperto that interface to enforce compile-time safety.
1635-1639: Type the empty Set to match Selection’s Key typeMinor TS nit:
new Set()defaults to a broad generic. Explicitly typing improves clarity and avoids potential type drift.- this.setState({ - selectedItemIds: new Set(), - isMetadataSidePanelOpen: false, - }); + this.setState({ + selectedItemIds: new Set<React.Key>(), + isMetadataSidePanelOpen: false, + });
1647-1651: Toggle/close methods are straightforwardImplementation is concise. Optional: guard toggle when no selection is present to align with UI affordance and avoid accidental opens via programmatic calls.
Also applies to: 1659-1661
1781-1784: Header render condition should track current view, not default viewUsing
defaultViewto decide Header visibility is static and may hide the Header even after navigating away from metadata view. Prefer checking the currentview.- {!isDefaultViewMetadata && ( + {view !== VIEW_METADATA && ( <Header view={view} logoUrl={logoUrl} onSearch={this.search} /> )}Please verify there’s no product requirement to always hide Header when
defaultViewis metadata. If the intent is to hide Header only in metadata view, the change above brings behavior in line.
1785-1810: SubHeader wiring for panel toggle and selection is good; consider exposing open stateProps threading looks good. Optional: also pass
isMetadataSidePanelOpenso the toggle control can reflect pressed state (e.g., aria-pressed) for better accessibility feedback.
1856-1868: Tighten side panel render condition and add landmark semanticsTo prevent the panel from appearing outside metadata view or during error view, gate on the current
viewand!isErrorView. Also, add landmark roles to improve accessibility.- {isDefaultViewMetadata && - isMetadataViewV2Feature && - this.state.isMetadataSidePanelOpen && ( - <div className="bce-sidepanel"> + {view === VIEW_METADATA && + isMetadataViewV2Feature && + !isErrorView && + this.state.isMetadataSidePanelOpen && ( + <div className="bce-sidepanel" role="complementary" aria-label="Metadata"> <MetadataSidePanel currentCollection={currentCollection} closeMetadataSidePanel={this.closeMetadataSidePanel} metadataTemplate={metadataTemplate} selectedItemIds={this.state.selectedItemIds} /> </div> )}Optional (related): add
role="main"to<div className="bce-main">to establish landmarks clearly.
1780-1781: Add a main landmark to the primary content containerMinor a11y improvement: add
role="main"to the main region.- <div className="bce-container"> - <div className="bce-main"> + <div className="bce-container"> + <div className="bce-main" role="main">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
src/elements/common/sub-header/SubHeader.tsx(3 hunks)src/elements/common/sub-header/SubHeaderLeftV2.tsx(2 hunks)src/elements/common/sub-header/SubHeaderRight.tsx(5 hunks)src/elements/content-explorer/ContentExplorer.scss(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(7 hunks)src/elements/content-explorer/MetadataSidePanel.scss(1 hunks)src/elements/content-explorer/MetadataSidePanel.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(3 hunks)src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx(1 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)src/elements/content-explorer/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/elements/content-explorer/MetadataSidePanel.scss
- src/elements/content-explorer/ContentExplorer.scss
- src/elements/common/sub-header/SubHeaderLeftV2.tsx
- src/elements/common/sub-header/SubHeader.tsx
- src/elements/content-explorer/MetadataSidePanel.tsx
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
- src/elements/common/sub-header/SubHeaderRight.tsx
- src/elements/content-explorer/utils.ts
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
- src/elements/content-explorer/tests/MetadataSidePanel.test.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/elements/content-explorer/ContentExplorer.tsx (2)
src/elements/content-picker/Content.js (1)
Content(51-98)src/features/metadata-based-view/__tests__/MetadataBasedItemList.test.js (1)
fieldsToShow(102-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Summary
- GitHub Check: lint_test_build
🔇 Additional comments (5)
src/elements/content-explorer/ContentExplorer.tsx (5)
27-27: Import of MetadataSidePanel looks correctNew component import is clear and scoped to Content Explorer.
173-173: State wiring forisMetadataSidePanelOpenis solidType and initial value are added consistently in State and constructor.
Also applies to: 299-301
1760-1760: Feature flag gate is correctly computedReads cleanly and centralizes the V2 feature enablement.
1811-1839: Content props extension for metadata view integration looks consistentPassing
metadataTemplateandmetadataViewPropsinto Content aligns with the new side panel flow. No concerns here.
1841-1855: Footer/Pagination placement under the new layout is appropriateConditional render on error view preserves existing behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
1568-1572: Fix TS compile error from conditional spread in setStateSpreading a boolean when
isSelectionEmptyis false is a TS error (“Spread types may only be created from object types”). Use a ternary to always spread an object.Apply this diff:
const isSelectionEmpty = ids !== 'all' && ids.size === 0; this.setState({ selectedItemIds: ids, - ...(isSelectionEmpty && { isMetadataSidePanelOpen: false }), + ...(isSelectionEmpty ? { isMetadataSidePanelOpen: false } : {}), });
🧹 Nitpick comments (3)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
178-191: Harden the play steps to reduce flakiness and assert the panel openedUse async querying for the Metadata button and add an assertion that the side panel actually opened after the click.
Apply this diff:
play: async ({ canvas }) => { await waitFor(() => { expect(canvas.getByRole('row', { name: /Child 2/i })).toBeInTheDocument(); }); // Select the first row by clicking its checkbox const firstRow = canvas.getByRole('row', { name: /Child 2/i }); const checkbox = within(firstRow).getByRole('checkbox'); await userEvent.click(checkbox); - const metadataButton = canvas.getByRole('button', { name: 'Metadata' }); - await userEvent.click(metadataButton); + const metadataButton = await canvas.findByRole('button', { name: /metadata/i }); + await userEvent.click(metadataButton); + + // Assert that the side panel opened + await waitFor(() => { + expect(document.querySelector('.bce-sidepanel')).toBeInTheDocument(); + }); },src/elements/content-explorer/ContentExplorer.tsx (2)
1855-1866: Gate side panel by current view, add ARIA for testability/a11yRendering should likely depend on the active view (metadata) rather than the initial defaultView. Also, add a landmark role and label so tests and screen readers can target the side panel reliably.
Apply this diff:
- {isDefaultViewMetadata && - isMetadataViewV2Feature && - this.state.isMetadataSidePanelOpen && ( - <div className="bce-sidepanel"> + {view === VIEW_METADATA && + isMetadataViewV2Feature && + this.state.isMetadataSidePanelOpen && ( + <div + className="bce-sidepanel" + role="complementary" + aria-label="Metadata side panel" + >Follow-up:
- Verify whether the intent is to only show the side panel during metadata view. If yes, the change above aligns behavior with user expectations and your story’s flow.
1666-1670: Guard toggle to avoid opening panel with empty selectionDefensive check prevents opening the panel programmatically when no items are selected. The UI button is gated, but this keeps state consistent if methods are called directly.
toggleMetadataSidePanel = () => { - this.setState(prevState => ({ - isMetadataSidePanelOpen: !prevState.isMetadataSidePanelOpen, - })); + const { selectedItemIds } = this.state; + const isSelectionEmpty = selectedItemIds !== 'all' && selectedItemIds.size === 0; + if (isSelectionEmpty) { + return; + } + this.setState(prevState => ({ + isMetadataSidePanelOpen: !prevState.isMetadataSidePanelOpen, + })); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/elements/content-explorer/ContentExplorer.tsx(7 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/elements/content-explorer/ContentExplorer.tsx (2)
src/elements/content-sidebar/versions/VersionsItem.js (1)
canPreview(108-108)src/features/metadata-based-view/__tests__/MetadataBasedItemList.test.js (1)
fieldsToShow(102-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (5)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
167-177: Story args look good and align with the new flowReusing metadataViewV2ElementProps, enabling row selection, and keeping columns consistent with v2 is appropriate for this visual scenario.
src/elements/content-explorer/ContentExplorer.tsx (4)
1654-1658: Good: clearing selection also closes the side panelThis keeps the UI consistent with the requirement that the metadata side panel is actionable only with a selection.
1758-1758: Feature flag guard is clean and centralizedUsing isFeatureEnabled for the v2 path is consistent with the rest of the codebase and reads clearly.
1777-1807: SubHeader wiring looks correctPassing selectedItemIds and onToggleMetadataSidePanel into SubHeader enables the “Metadata” button visibility and action. The new container structure (.bce-container/.bce-main) appears cohesive.
1809-1837: Content props integration is coherentProviding metadataTemplate, metadataViewProps, and onMetadataUpdate ties the v2 metadata flow together. No issues spotted.
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/elements/content-explorer/ContentExplorer.tsx (1)
1568-1572: Fix TS compile error: avoid spreading a boolean in setState
...(isSelectionEmpty && { ... })evaluates tofalsewhen not empty, which TypeScript disallows spreading. Use a ternary that always yields an object.Apply this diff:
- const isSelectionEmpty = ids !== 'all' && ids.size === 0; - this.setState({ - selectedItemIds: ids, - ...(isSelectionEmpty && { isMetadataSidePanelOpen: false }), - }); + const isSelectionEmpty = ids !== 'all' && ids.size === 0; + this.setState({ + selectedItemIds: ids, + ...(isSelectionEmpty ? { isMetadataSidePanelOpen: false } : {}), + });src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (3)
104-109: Fix typo: “calls”Make the test title grammatically correct and align with the updated label.
Apply this diff:
- test('call closeMetadataSidePanel when close button is clicked', async () => { + test('calls closeMetadataSidePanel when close button is clicked', async () => { renderComponent(); - const closeButton = screen.getByLabelText('Clear selection'); + const closeButton = screen.getByLabelText('Close'); await userEvent.click(closeButton); expect(mockCloseMetadataSidePanel).toHaveBeenCalledTimes(1); });
111-115: Fix typo: “renders correct subtitle”Update the test description.
Apply this diff:
- test('render correct subtitle when multiple items are selected', () => { + test('renders correct subtitle when multiple items are selected', () => { renderComponent({ selectedItemIds: new Set(['1', '2']) }); const subtitle = screen.getByText('2 files selected'); expect(subtitle).toBeInTheDocument(); });
117-126: Fix typo: “renders cancel and submit”Update the test description for grammar.
Apply this diff:
- test('render cancel and submit button when in edit mode', async () => { + test('renders cancel and submit button when in edit mode', async () => { renderComponent(); const editTemplateButton = screen.getByLabelText('Edit Mock Template'); await userEvent.click(editTemplateButton); const cancelButton = screen.getByRole('button', { name: 'Cancel' }); expect(cancelButton).toBeInTheDocument(); const submitButton = screen.getByRole('button', { name: 'Save' }); expect(submitButton).toBeInTheDocument(); });src/elements/content-explorer/MetadataSidePanel.tsx (3)
41-47: Defensively compute template instance to avoid runtime errors
getTemplateInstancedereferences nested metadata; if selection is empty or the first item lacks the template, it will throw. Guard the call and fall back tonull.Apply this diff:
- const templateInstance = getTemplateInstance(metadataTemplate, selectedItems); + const hasTemplateData = + selectedItems.length > 0 && + Boolean(selectedItems[0]?.metadata?.[metadataTemplate.scope]?.[metadataTemplate.templateKey]); + const templateInstance = hasTemplateData ? getTemplateInstance(metadataTemplate, selectedItems) : null;
93-124: Render metadata components only when a valid template instance exists; prefer undefined over null for optional propsAvoid rendering the editor/view when
templateInstanceis null and useundefinedinstead ofnullfor optional function props to satisfy stricter typings.Apply this diff:
- <div className="bce-MetadataSidePanel-content"> - <AutofillContextProvider fetchSuggestions={null} isAiSuggestionsFeatureEnabled={false}> - {isEditing ? ( + <div className="bce-MetadataSidePanel-content"> + <AutofillContextProvider isAiSuggestionsFeatureEnabled={false}> + {templateInstance && (isEditing ? ( <MetadataInstanceForm areAiSuggestionsAvailable={false} isAiSuggestionsFeatureEnabled={false} isBetaLanguageEnabled={false} isDeleteButtonDisabled={true} isDeleteConfirmationModalCheckboxEnabled={false} isLargeFile={false} isMultilevelTaxonomyFieldEnabled={false} isUnsavedChangesModalOpen={isUnsavedChangesModalOpen} selectedTemplateInstance={templateInstance} onCancel={handleMetadataInstanceFormCancel} onChange={handleMetadataInstanceFormChange} - onDelete={null} + onDelete={undefined} onDiscardUnsavedChanges={handleMetadataInstanceFormDiscardUnsavedChanges} onSubmit={handleMetadataInstanceFormSubmit} setIsUnsavedChangesModalOpen={setIsUnsavedChangesModalOpen} - taxonomyOptionsFetcher={null} + taxonomyOptionsFetcher={undefined} /> - ) : ( + ) : ( <MetadataInstance areAiSuggestionsAvailable={false} isAiSuggestionsFeatureEnabled={false} isBetaLanguageEnabled={false} onEdit={handleMetadataInstanceEdit} templateInstance={templateInstance} - taxonomyNodeFetcher={null} + taxonomyNodeFetcher={undefined} /> - )} + )} </AutofillContextProvider> </div>
85-90: A11y: Update “Close” button aria-labelThe X button currently uses the “Clear selection” label but actually closes the panel. Replace the
clearSelectionkey with the existingclosekey:< IconButton - aria-label={formatMessage(messages.clearSelection)} + aria-label={formatMessage(messages.close)} icon={XMark} onClick={onClose} size="large" />The
closekey is already defined insrc/elements/common/messages.js, so no additional localization is needed.
🧹 Nitpick comments (6)
src/elements/content-explorer/ContentExplorer.tsx (2)
1666-1670: Guard toggle against empty selectionOptional defensive check so the panel can’t be opened with an empty selection even if a caller misfires. Keeps behavior aligned with the button’s visibility logic.
Apply this diff:
- toggleMetadataSidePanel = () => { - this.setState(prevState => ({ - isMetadataSidePanelOpen: !prevState.isMetadataSidePanelOpen, - })); - }; + toggleMetadataSidePanel = () => { + this.setState(prevState => { + const isSelectionEmpty = + prevState.selectedItemIds !== 'all' && prevState.selectedItemIds.size === 0; + return isSelectionEmpty + ? { isMetadataSidePanelOpen: false } + : { isMetadataSidePanelOpen: !prevState.isMetadataSidePanelOpen }; + }); + };
1856-1862: Render gate likely should key off current view, not defaultViewUsing
isDefaultViewMetadatameans the side panel only renders when the initial propdefaultViewis metadata, even if the user is currently in metadata view. If the intent is “panel in metadata view,” checkview === VIEW_METADATA.Apply this diff:
- {isDefaultViewMetadata && isMetadataViewV2Feature && isMetadataSidePanelOpen && ( + {view === VIEW_METADATA && isMetadataViewV2Feature && isMetadataSidePanelOpen && (If this was intentional, please confirm the UX requirement (panel only in default metadata view) and ignore.
src/elements/content-explorer/MetadataSidePanel.scss (1)
6-12: Avoid brittle targeting by data-target-id where possibleHiding
[data-target-id='TextButton-deleteButton']can be fragile if the internal component structure changes. Prefer explicit props to disable delete actions if the metadata editor exposes them (you’re already usingisDeleteButtonDisabledfor the form). If the view component lacks such a prop, consider a follow-up to add one rather than relying on DOM targeting.I can help check the metadata-editor surface for a supported prop or sketch an API addition if needed.
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
489-507: Consider asserting panel open after clicking “Metadata”You validate the button appears after selection; adding an assertion that clicking it renders the side panel would increase confidence in the integration.
I can add a follow-up test that clicks “Metadata” and expects the panel title (“Metadata”) and selected item text to be visible.
src/elements/content-explorer/MetadataSidePanel.tsx (2)
79-83: A11y: mark decorative icon as hiddenThe file icon is purely visual; add
aria-hiddenso it isn’t announced by screen readers.Apply this diff:
- <FileDefault /> + <FileDefault aria-hidden="true" />
56-69: Track TODOs: onChange/onSubmit multi-select supportOpen items remain:
- Implement form onChange/onSubmit
- Support displaying values for multiple selected items
Flagging to ensure they’re tracked before feature rollout.
I can help scaffold the onSubmit flow against metadata-query APIs and outline a multi-select merge strategy (e.g., show “mixed” states with partial values).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/elements/common/sub-header/SubHeaderRight.tsx(5 hunks)src/elements/content-explorer/ContentExplorer.scss(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(9 hunks)src/elements/content-explorer/MetadataSidePanel.scss(1 hunks)src/elements/content-explorer/MetadataSidePanel.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(3 hunks)src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/common/sub-header/SubHeaderRight.tsx
- src/elements/content-explorer/ContentExplorer.scss
🧰 Additional context used
🧠 Learnings (2)
📚 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-explorer/__tests__/MetadataSidePanel.test.tsx
📚 Learning: 2025-06-25T13:10:19.128Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:11-11
Timestamp: 2025-06-25T13:10:19.128Z
Learning: In the box-ui-elements codebase, Flow files support `import type` syntax for type-only imports. The `import type` statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by flow comments).
Applied to files:
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx
🧬 Code Graph Analysis (3)
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (2)
src/elements/content-explorer/MetadataSidePanel.tsx (1)
MetadataSidePanelProps(24-29)src/elements/content-explorer/ContentExplorer.tsx (1)
render(1689-1960)
src/elements/content-explorer/MetadataSidePanel.tsx (1)
src/elements/content-explorer/utils.ts (2)
useSelectedItemText(11-29)getTemplateInstance(32-58)
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (3)
src/elements/content-explorer/ContentExplorer.tsx (1)
ContentExplorer(1963-1963)src/elements/common/__mocks__/mockMetadata.ts (1)
mockSchema(226-226)src/features/metadata-based-view/__tests__/MetadataBasedItemList.test.js (1)
fieldsToShow(102-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (4)
src/elements/content-explorer/ContentExplorer.tsx (2)
1654-1658: Good: clearing selection also closes the panelResetting
selectedItemIdsand closing the side panel together keeps UI state consistent.
1779-1854: Nice layout consolidation and SUIT namingThe
bce-ContentExplorer-mainwrapper and threaded props (selection state, toggle handlers) are clean and align with the SUIT convention previously requested.src/elements/content-explorer/MetadataSidePanel.scss (1)
1-4: SCSS looks goodSubtitle container styles are straightforward and local.
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
82-88: Good: pass features through FeatureProvider and component propsPropagating
featuresconsistently allows exercising the v2 codepaths in tests.
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
1566-1573: Fix conditional spread that can spread a boolean (TS compile error)The pattern ...(isSelectionEmpty && { ... }) spreads a boolean when false. TypeScript rejects spreading non-object values.
Apply this diff:
- const isSelectionEmpty = ids !== 'all' && ids.size === 0; - this.setState({ - selectedItemIds: ids, - ...(isSelectionEmpty && { isMetadataSidePanelOpen: false }), - }); + const isSelectionEmpty = ids !== 'all' && ids.size === 0; + this.setState({ + selectedItemIds: ids, + ...(isSelectionEmpty ? { isMetadataSidePanelOpen: false } : {}), + });
🧹 Nitpick comments (4)
src/elements/common/sub-header/SubHeaderRight.tsx (2)
68-68: Use a TS-narrowing-friendly condition for hasSelectedItemsThe current OR check doesn’t narrow selectedItemIds to Set on the right-hand side, which can surface TS errors for .size. Prefer instanceof Set or an &&-based narrowing to keep types sound and avoid relying on non-narrowing OR semantics.
Apply this diff:
- const hasSelectedItems: boolean = !!(selectedItemIds && (selectedItemIds === 'all' || selectedItemIds.size > 0)); + const hasSelectedItems = + selectedItemIds === 'all' || (selectedItemIds instanceof Set && selectedItemIds.size > 0);
99-103: Guard against missing click handler or make it explicitButton renders gated by feature flag and selection, but onMetadataSidePanelToggle is optional. If this component is reused elsewhere, a missing handler yields a no-op button. Either gate on the presence of the handler or default it to a noop to make intent explicit.
Example options:
- Gate rendering:
-{isMetadataView && isMetadataViewV2Feature && hasSelectedItems && ( +{isMetadataView && isMetadataViewV2Feature && hasSelectedItems && onMetadataSidePanelToggle && (
- Or default in destructuring:
- onMetadataSidePanelToggle, + onMetadataSidePanelToggle = () => {},src/elements/content-explorer/ContentExplorer.tsx (2)
1760-1761: Nit: compute and reuse isMetadataViewFor readability and consistency with SubHeaderRight, consider computing isMetadataView (view === VIEW_METADATA) once and reusing it in render conditions.
Example:
- const isDefaultViewMetadata: boolean = defaultView === DEFAULT_VIEW_METADATA; - const isMetadataViewV2Feature = isFeatureEnabled(features, 'contentExplorer.metadataViewV2'); + const isDefaultViewMetadata = defaultView === DEFAULT_VIEW_METADATA; + const isMetadataViewV2Feature = isFeatureEnabled(features, 'contentExplorer.metadataViewV2'); + const isMetadataView = view === VIEW_METADATA;
1855-1862: Gate side panel on current view, not default viewRendering the panel based on defaultView can keep it visible when the user navigates away from metadata view. Tie it to the current view instead, so the panel only shows in VIEW_METADATA.
Apply this diff:
- {isDefaultViewMetadata && isMetadataViewV2Feature && isMetadataSidePanelOpen && ( + {view === VIEW_METADATA && isMetadataViewV2Feature && isMetadataSidePanelOpen && ( <MetadataSidePanel currentCollection={currentCollection} onClose={this.closeMetadataSidePanel} metadataTemplate={metadataTemplate} selectedItemIds={selectedItemIds} /> )}Optionally, also close the panel when leaving metadata view to keep state consistent:
- } else if (view === VIEW_METADATA) { + } else if (view === VIEW_METADATA) { this.showMetadataQueryResults(); + } else if (this.state.isMetadataSidePanelOpen) { + this.setState({ isMetadataSidePanelOpen: false }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/elements/common/sub-header/SubHeader.tsx(3 hunks)src/elements/common/sub-header/SubHeaderRight.tsx(5 hunks)src/elements/content-explorer/ContentExplorer.tsx(9 hunks)src/elements/content-explorer/MetadataSidePanel.tsx(1 hunks)src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/elements/content-explorer/MetadataSidePanel.tsx
- src/elements/content-explorer/tests/MetadataSidePanel.test.tsx
- src/elements/common/sub-header/SubHeader.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (10)
src/elements/common/sub-header/SubHeaderRight.tsx (3)
5-5: LGTM: correct type-only import for SelectionType-only import from react-aria-components is appropriate and keeps bundles lean.
31-31: LGTM: new toggle handler prop is well-scopedOptional onMetadataSidePanelToggle prop looks good and matches usage downstream.
35-35: LGTM: wiring selectedItemIds as SelectionProp aligns with react-aria Selection union and supports 'all' and Set cases.
src/elements/content-explorer/ContentExplorer.tsx (7)
27-27: LGTM: side panel importImporting MetadataSidePanel here is expected; keeps ContentExplorer the integration point.
173-173: LGTM: add isMetadataSidePanelOpen to stateState flag is clearly named and scoped.
299-299: LGTM: initialize panel closedDefaulting isMetadataSidePanelOpen to false is correct.
1654-1658: LGTM: clear selection also closes panelGood UX to close the panel when selection is cleared.
1666-1670: LGTM: toggle handlerSimple and side-effect free toggle implementation. Consider also closing on navigation away from metadata view (see next comment).
1780-1808: LGTM: header/subheader/prop threading and new main wrapperThe new bce-ContentExplorer-main wrapper and threading of onMetadataSidePanelToggle and selectedItemIds through SubHeader look consistent with the new panel behavior.
1555-1576: Rename verified – no remainingonToggleMetadataSidePanelreferences
Ripgrep confirmed only the newonMetadataSidePanelToggleoccurrences in the following locations:
- src/elements/content-explorer/ContentExplorer.tsx (method definition and prop usage)
- src/elements/common/sub-header/SubHeader.tsx (interface and JSX)
- src/elements/common/sub-header/SubHeaderRight.tsx (interface and JSX)
No action needed.
* feat(metadata-view): Implement metadata sidepanel * feat(metadata-view): resolve comments * feat(metadata-view): resolve comments * feat(metadata-view): resolve nits
* feat(timestamped-comments): feature enabled timesetamp support * feat(timestamped-comments): updating stories and adding toggle form * feat(timestamped-comments): fixing flow errors * feat(timestamped-comments): removing unnecessary prop * feat(timestamped-comments): removing formatting * feat(timestamped-comments): removing async modifier * feat(timestamped-comments): removing unnecssary import * feat(timestamped-comments): passing down file prop to BaseComment * feat(metadata-instance-editor): remove ai agent selector split and fi… (#4226) * feat(metadata-instance-editor): remove ai agent selector split and fix styling * fix: tests * fix: tests * feat: use new box ai agent selector with alignment * feat(metadata-view): Implement metadata sidepanel (#4230) * feat(metadata-view): Implement metadata sidepanel * feat(metadata-view): resolve comments * feat(metadata-view): resolve comments * feat(metadata-view): resolve nits * feat(timestamped-comments): addressing PR feedback * feat(timestamped-comments): addressing PR comments * feat(timestamped-comments): addressing PR feedback * feat(timestamped-comments): addressing PR feedback * feat(timestamped-comments): fixing flow error * feat(timestamped-comments): addressing PR feedback * feat(timestamped-comments): fixing unit tests --------- Co-authored-by: jlandisBox <[email protected]> Co-authored-by: Jerry Jiang <[email protected]>
…#4228) * feat(timestamped-comments): feature enabled timesetamp support * feat(timestamped-comments): updating stories and adding toggle form * feat(timestamped-comments): fixing flow errors * feat(timestamped-comments): removing unnecessary prop * feat(timestamped-comments): removing formatting * feat(timestamped-comments): removing async modifier * feat(timestamped-comments): removing unnecssary import * feat(timestamped-comments): passing down file prop to BaseComment * feat(metadata-instance-editor): remove ai agent selector split and fi… (box#4226) * feat(metadata-instance-editor): remove ai agent selector split and fix styling * fix: tests * fix: tests * feat: use new box ai agent selector with alignment * feat(metadata-view): Implement metadata sidepanel (box#4230) * feat(metadata-view): Implement metadata sidepanel * feat(metadata-view): resolve comments * feat(metadata-view): resolve comments * feat(metadata-view): resolve nits * feat(timestamped-comments): addressing PR feedback * feat(timestamped-comments): addressing PR comments * feat(timestamped-comments): addressing PR feedback * feat(timestamped-comments): addressing PR feedback * feat(timestamped-comments): fixing flow error * feat(timestamped-comments): addressing PR feedback * feat(timestamped-comments): fixing unit tests --------- Co-authored-by: jlandisBox <[email protected]> Co-authored-by: Jerry Jiang <[email protected]>
Included Changes:
TODO:
Landing Page
At least one Item is selected - Metadata Button gets displayed
Metadata SidePanel ViewMode - after Metadata button is clicked
Metadata SidePanel EditMode - after edit button is clicked
Summary by CodeRabbit
New Features
Refactor
Style
Tests