feat(metadata-view): Add Subheader for Metadata View v2 #4202
Conversation
WalkthroughThis change introduces multi-selection support and dynamic folder naming to the Content Explorer. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ContentExplorer
participant SubHeader
participant SubHeaderLeftV2
User->>ContentExplorer: Selects/deselects items
ContentExplorer->>ContentExplorer: Updates selectedItemIds state
ContentExplorer->>SubHeader: Passes selectedItemIds, title, onClearSelectedIds
SubHeader->>SubHeaderLeftV2: Renders with selection props (if feature enabled)
SubHeaderLeftV2->>User: Displays selection state or title, allows clearing selection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ 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)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (6)
src/elements/common/sub-header/SubHeaderLeftMetadataViewV2.scss (1)
1-5: Consider using CSS custom properties for consistent font sizing.The font styles look good, but consider using CSS custom properties or design system tokens for font sizes and weights to maintain consistency across the application.
.be-sub-header-left-title { font-family: Lato, sans-serif; - font-size: 21px; - font-weight: 900; + font-size: var(--font-size-large, 21px); + font-weight: var(--font-weight-heavy, 900); }src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
83-83: Remove debug statement before production.The
screen.debug(null, 10000)statement should be removed as it's only useful during development and can clutter test output.test('should render MetadataView component', () => { renderComponent(); - screen.debug(null, 10000); expect(screen.getByRole('button', { name: 'All Filters' })).toBeInTheDocument();src/elements/common/sub-header/SubHeaderLeftMetadataViewV2.tsx (1)
19-19: Consider extracting the complex ternary expression for better readability.The nested ternary operation for calculating
selectedCountcould be more readable if extracted into a helper function or simplified.- const selectedCount = selectedKeys === 'all' ? currentCollection.items.length : selectedKeys.size; + const selectedCount = selectedKeys === 'all' + ? currentCollection.items.length + : selectedKeys.size;src/elements/content-explorer/MetadataViewContainer.tsx (1)
27-35: Consider sanitizing field.key for filter ID generation.The filter ID uses
${field.key}-filterwhich could cause issues iffield.keycontains special characters or spaces. Consider sanitizing or encoding the key.return { - id: `${field.key}-filter`, + id: `${field.key.replace(/[^a-zA-Z0-9-_]/g, '_')}-filter`, name: field.displayName, fieldType: field.type, options: field.options?.map(({ key }) => key) || [], shouldRenderChip: true, };src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
72-78: Consider making the padding value configurable.The hardcoded 50px padding could be made configurable through story args for better flexibility in visual testing.
- render: args => { + render: args => { + const padding = args.padding || '50px'; return ( - <div style={{ padding: '50px' }}> + <div style={{ padding }}> <ContentExplorer {...args} /> </div> ); },src/elements/content-explorer/ContentExplorer.tsx (1)
1689-1700: Consider extracting the metadata props merging logic for better readability.The deeply nested spread operations make the code harder to read and maintain. Consider extracting this logic into a helper function.
- const combinedMetadataProps = { - ...metadataProps, - tableProps: { - ...metadataProps?.tableProps, - selectedKeys: this.state.selectedKeys, - onSelectionChange: (keys: Selection) => { - console.log('onSelectionChange', { keys }); - metadataProps?.tableProps?.onSelectionChange?.(keys); - this.setState({ selectedKeys: keys }); - }, - }, - }; + const combinedMetadataProps = this.mergeMetadataProps(metadataProps);Add this helper method to the class:
private mergeMetadataProps = (metadataProps?: Omit<MetadataViewContainerProps, 'hasError' | 'currentCollection'>) => { return { ...metadataProps, tableProps: { ...metadataProps?.tableProps, selectedKeys: this.state.selectedKeys, onSelectionChange: (keys: Selection) => { metadataProps?.tableProps?.onSelectionChange?.(keys); this.setState({ selectedKeys: keys }); }, }, }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (20)
package.json(5 hunks)scripts/i18n.config.js(1 hunks)scripts/jest/jest.config.js(1 hunks)src/common/types/core.js(2 hunks)src/elements/common/__mocks__/mockMetadata.ts(3 hunks)src/elements/common/sub-header/SubHeader.scss(1 hunks)src/elements/common/sub-header/SubHeader.tsx(5 hunks)src/elements/common/sub-header/SubHeaderLeftMetadataViewV2.scss(1 hunks)src/elements/common/sub-header/SubHeaderLeftMetadataViewV2.tsx(1 hunks)src/elements/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(15 hunks)src/elements/content-explorer/MetadataView.tsx(0 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(1 hunks)src/elements/content-explorer/__tests__/Content.test.tsx(3 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(1 hunks)src/elements/content-explorer/stories/MetadataView.stories.tsx(1 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(4 hunks)src/features/metadata-based-view/MetadataQueryAPIHelper.js(4 hunks)src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/elements/content-explorer/MetadataView.tsx
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: ahorowitz123
PR: box/box-ui-elements#4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, `isExistingAIExtractionCascadePolicy` specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: `VersionSidebarView` intentionally uses the `versionId` field to stay consistent with current URL parameter naming; a potential rename to `fileVersionId` is deferred until React Router is removed.
scripts/i18n.config.js (3)
Learnt from: tjuanitas
PR: #4126
File: scripts/buildTranslations.js:1-8
Timestamp: 2025-06-17T15:16:46.279Z
Learning: The buildTranslations and buildLanguageBundles functions from @box/frontend package are synchronous functions that already handle errors internally, so additional error handling wrappers and await keywords are not needed.
Learnt from: jpan-box
PR: #4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Learnt from: tjuanitas
PR: #4126
File: .storybook/reactIntl.ts:4-7
Timestamp: 2025-06-17T15:23:50.959Z
Learning: The @box/languages package exports an array of language codes as its default export, not an object. It can be used directly with array methods like .reduce().
scripts/jest/jest.config.js (2)
Learnt from: jpan-box
PR: #4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Learnt from: tjuanitas
PR: #4126
File: scripts/webpack.config.js:72-76
Timestamp: 2025-06-17T15:21:36.180Z
Learning: The Box UI Elements project does not run webpack builds on Windows machines, so Windows path separator compatibility is not a concern for their build scripts.
src/elements/content-explorer/Content.tsx (2)
Learnt from: ahorowitz123
PR: #4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, isExistingAIExtractionCascadePolicy specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
Learnt from: rafalmaksymiuk
PR: #4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: VersionSidebarView intentionally uses the versionId field to stay consistent with current URL parameter naming; a potential rename to fileVersionId is deferred until React Router is removed.
src/elements/common/sub-header/SubHeader.tsx (3)
Learnt from: rafalmaksymiuk
PR: #4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: VersionSidebarView intentionally uses the versionId field to stay consistent with current URL parameter naming; a potential rename to fileVersionId is deferred until React Router is removed.
Learnt from: rafalmaksymiuk
PR: #4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Learnt from: rafalmaksymiuk
PR: #4160
File: src/elements/content-sidebar/SidebarToggle.js:21-27
Timestamp: 2025-06-25T13:09:54.538Z
Learning: The box-ui-elements project uses Flow for type annotations in JavaScript files, as indicated by @flow directives in file headers. Type annotations like : Props are valid Flow syntax, not TypeScript syntax.
src/elements/content-explorer/ContentExplorer.tsx (2)
Learnt from: ahorowitz123
PR: #4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, isExistingAIExtractionCascadePolicy specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
Learnt from: rafalmaksymiuk
PR: #4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
package.json (3)
Learnt from: rafalmaksymiuk
PR: #4160
File: src/elements/content-sidebar/SidebarToggle.js:21-27
Timestamp: 2025-06-25T13:09:54.538Z
Learning: The box-ui-elements project uses Flow for type annotations in JavaScript files, as indicated by @flow directives in file headers. Type annotations like : Props are valid Flow syntax, not TypeScript syntax.
Learnt from: jpan-box
PR: #4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Learnt from: tjuanitas
PR: #4126
File: scripts/webpack.config.js:72-76
Timestamp: 2025-06-17T15:21:36.180Z
Learning: The Box UI Elements project does not run webpack builds on Windows machines, so Windows path separator compatibility is not a concern for their build scripts.
src/elements/common/sub-header/SubHeaderLeftMetadataViewV2.tsx (1)
Learnt from: rafalmaksymiuk
PR: #4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: VersionSidebarView intentionally uses the versionId field to stay consistent with current URL parameter naming; a potential rename to fileVersionId is deferred until React Router is removed.
🧬 Code Graph Analysis (1)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (2)
src/elements/common/__mocks__/mockMetadata.ts (1)
mockSchema(226-226)src/elements/content-explorer/ContentExplorer.tsx (1)
ContentExplorer(1878-1878)
🪛 Biome (2.1.2)
src/common/types/core.js
[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/features/metadata-based-view/MetadataQueryAPIHelper.js
[error] 57-57: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 59-59: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 59-59: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 163-163: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 163-163: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 171-171: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 171-171: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 171-171: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 174-174: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (36)
src/elements/common/sub-header/SubHeader.scss (1)
23-26: LGTM: Clean CSS modifier for metadata view styling.The new
.be-sub-header--metadata-viewmodifier follows BEM conventions and appropriately removes padding to support the new metadata view layout.scripts/i18n.config.js (1)
8-9: LGTM: Necessary translation dependencies for metadata packages.Adding the new metadata packages to the translation dependencies ensures proper internationalization support for the metadata view V2 features.
src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js (1)
189-189: LGTM: Test fixture updated to match API changes.The addition of
metadataTemplateto the test fixture correctly reflects the enhanced data structure returned bygetFlattenedDataWithTypesin the V2 implementation.src/common/types/core.js (2)
397-397: LGTM: Collection type enhanced for metadata template support.The addition of the optional
metadataTemplateproperty to the Collection type correctly supports the metadata view V2 functionality and aligns with the API changes.
37-37: Import type syntax is valid Flow and in use—no action neededThe
import type { … } from '…';pattern appears in many Flow-annotated.jsfiles (e.g.,ToggleField.js,TextInputField.js), and a.flowconfigexists. The static analysis error is a false positive—Flow supports type-only imports in JavaScript files.scripts/jest/jest.config.js (1)
29-29: LGTM: Jest configuration updated for new metadata packages.Adding the new
@box/metadata-filter,@box/metadata-view, and@box/typespackages to the transform exceptions ensures Jest properly processes these dependencies during testing.src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (2)
413-413: Test assertion updated correctly for new mock data.The test now checks for "Child 2 of metadata folder.pdf" which matches the first entry in the updated mock metadata structure.
32-34: Confirmed: All tests now use a single metadata entry
- The only reference to
mockMetadata.entriesis in
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx, where it’s correctly limited toentries[0].- No other tests in the repository reference multiple entries or expect more than one item.
No further changes are needed.
src/elements/common/sub-header/SubHeaderLeftMetadataViewV2.scss (1)
7-18: Well-structured flexbox layout with appropriate specificity.The flexbox layout with gap and center alignment is appropriate for selection UI. The nested selector for
.bdl-CloseButtoncorrectly removes unwanted margins without being overly specific.src/elements/content-explorer/__tests__/Content.test.tsx (3)
40-43: Simple mock implementation is appropriate for unit testing.The MetadataViewContainer mock returns a simple div with identifying text, which is sufficient for testing the conditional rendering logic in the Content component.
96-102: Good refactoring to reduce code duplication.Extracting the shared collection object eliminates duplication and makes the tests more maintainable.
123-123: Test assertion correctly updated for mocked component.The expectation now looks for "MetadataViewContainer" text, which matches the mock implementation.
src/elements/common/__mocks__/mockMetadata.ts (1)
4-24: Rich mock data structure supports comprehensive testing.The updated mock metadata provides detailed and realistic data for testing the enhanced metadata view functionality, including roles, industries, and versioning information.
package.json (4)
127-140: LGTM! Dependency updates support new metadata view features.The version updates and new package additions are appropriate for the metadata view V2 functionality. The @box/blueprint-web update from 12.7.1 to 12.41.0 is significant but aligns with the new UI requirements.
155-155: LGTM! Virtual scrolling dependency properly added.The @tanstack/react-virtual package is correctly added to support virtualization features in the new metadata view.
250-250: LGTM! Accessibility component dependency properly added.The react-aria-components package is correctly added to support accessible selection components in the metadata view V2 feature.
295-330: LGTM! PeerDependencies are consistent with dev dependencies.All new packages and version updates are properly reflected in peerDependencies, ensuring consumers will have the correct dependency versions.
src/elements/common/sub-header/SubHeader.tsx (3)
3-7: LGTM! New imports support metadata view V2 functionality.The imports are properly added to support dynamic styling, selection state typing, and the new metadata view component.
36-37: LGTM! New props properly typed for selection management.The selectedKeys and onClearSelectedKeys props are correctly typed and support the selection functionality in metadata view V2.
70-74: LGTM! Dynamic className enables feature-specific styling.The conditional CSS class application allows for proper styling differentiation when the metadata view V2 feature is active.
src/elements/content-explorer/Content.tsx (3)
7-7: LGTM! Import updated for new metadata view container.The import change from MetadataView to MetadataViewContainer aligns with the new metadata view V2 implementation.
39-39: LGTM! Well-designed prop interface for metadata container.The metadataProps typing correctly excludes currentCollection to avoid duplication while allowing flexible configuration of the MetadataViewContainer.
84-92: LGTM! Improved rendering logic delegates empty state handling.The removal of the
!isViewEmptycheck allows MetadataViewContainer to handle empty states internally, which is a better separation of concerns. The props passed are appropriate for the container's functionality.src/elements/content-explorer/stories/MetadataView.stories.tsx (3)
14-30: LGTM! Well-structured metadata query for demonstration.The metadata query configuration is appropriate for the story, with helpful commented examples showing how filtering could be implemented.
39-75: LGTM! Comprehensive column configuration with type-specific handling.The column mapping logic properly handles different field types with appropriate renderers, especially the date formatting for better user experience.
117-123: LGTM! Appropriate MSW handlers for metadata API mocking.The MSW handlers correctly mock the metadata query and schema endpoints, enabling the story to demonstrate functionality without backend dependencies.
src/features/metadata-based-view/MetadataQueryAPIHelper.js (5)
57-62: LGTM! Backward-compatible constructor enhancement.The isV2 flag addition maintains backward compatibility while enabling new functionality for the metadata view V2 feature.
159-160: LGTM! Conditional metadata flattening supports both modes.The V2 mode appropriately bypasses metadata flattening, allowing the new MetadataViewContainer to work with raw metadata structures while preserving existing behavior.
166-167: LGTM! V2 mode allows broader item type support.The conditional filtering enables V2 to handle various item types beyond just files, expanding the capabilities of the new metadata view.
178-178: LGTM! MetadataTemplate inclusion enriches collection data.Adding metadataTemplate to the returned collection provides valuable template information for the new MetadataViewContainer component.
247-255: LGTM! Additional fields enhance V2 metadata richness.The inclusion of extension and creation date fields in V2 mode provides more comprehensive item information for enhanced metadata view functionality.
src/elements/common/sub-header/SubHeaderLeftMetadataViewV2.tsx (1)
25-31: LGTM! Good defensive programming with optional chaining.The single selection case correctly handles both 'all' and Set selection types, and the optional chaining on line 30 properly handles cases where the item might not be found.
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
38-45: LGTM! Well-structured column configuration.The column mapping from
mockSchema.fieldsis properly structured with all necessary properties for the metadata view.src/elements/content-explorer/ContentExplorer.tsx (3)
425-433: Well-implemented sorting enhancement for metadata queries.The code properly adds sorting to metadata queries while preserving any existing
order_byentries using the spread operator. Good use of nullish coalescing for safety.
863-863: Good addition to enable sorting for metadata views.The condition properly extends sorting capability to metadata views, aligning with the new metadata view V2 feature.
1600-1602: LGTM! Clean implementation of selection clearing.The
handleClearSelectedKeysmethod is simple and correctly resets the selection state.
jfox16
changed the title
src/elements/common/sub-header/SubHeaderLeftMetadataViewV2.scss
Outdated
Show resolved
Hide resolved
|
we use conventional commit messages: https://www.conventionalcommits.org/en/v1.0.0 you don't have to do it for each commit but mainly the PR title |
jfox16
changed the title
jfox16
force-pushed
the
subheader-left-metadata-view-v2
branch
from
9def413 to
c1281fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
245-245: Consider usingundefinedas the default value for optional prop.For optional props, it's more idiomatic to use
undefinedrather than an empty string as the default value. This makes it clearer when the prop is not provided.- metadataViewTitle: '', + metadataViewTitle: undefined,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
i18n/en-US.properties(1 hunks)src/api/APIFactory.js(1 hunks)src/common/types/core.js(2 hunks)src/elements/common/messages.js(1 hunks)src/elements/common/sub-header/SubHeader.scss(1 hunks)src/elements/common/sub-header/SubHeader.tsx(6 hunks)src/elements/common/sub-header/SubHeaderLeftMetadataViewV2.scss(1 hunks)src/elements/common/sub-header/SubHeaderLeftMetadataViewV2.tsx(1 hunks)src/elements/common/sub-header/__tests__/SubHeaderLeftMetadataViewV2.test.tsx(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(16 hunks)src/features/metadata-based-view/MetadataQueryAPIHelper.js(4 hunks)src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- i18n/en-US.properties
🚧 Files skipped from review as they are similar to previous changes (5)
- src/elements/common/sub-header/SubHeader.scss
- src/features/metadata-based-view/tests/MetadataQueryAPIHelper.test.js
- src/elements/common/sub-header/SubHeaderLeftMetadataViewV2.scss
- src/elements/common/sub-header/SubHeader.tsx
- src/elements/common/sub-header/SubHeaderLeftMetadataViewV2.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ahorowitz123
PR: box/box-ui-elements#4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, `isExistingAIExtractionCascadePolicy` specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: `VersionSidebarView` intentionally uses the `versionId` field to stay consistent with current URL parameter naming; a potential rename to `fileVersionId` is deferred until React Router is removed.
src/elements/common/messages.js (1)
Learnt from: jpan-box
PR: #4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
src/elements/content-explorer/ContentExplorer.tsx (4)
Learnt from: ahorowitz123
PR: #4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, isExistingAIExtractionCascadePolicy specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
Learnt from: rafalmaksymiuk
PR: #4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Learnt from: jpan-box
PR: #4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Learnt from: rafalmaksymiuk
PR: #4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: VersionSidebarView intentionally uses the versionId field to stay consistent with current URL parameter naming; a potential rename to fileVersionId is deferred until React Router is removed.
src/features/metadata-based-view/MetadataQueryAPIHelper.js (1)
Learnt from: ahorowitz123
PR: #4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, isExistingAIExtractionCascadePolicy specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
🧬 Code Graph Analysis (1)
src/api/APIFactory.js (1)
src/api/Folder.js (4)
if(374-376)if(116-118)if(261-263)if(218-220)
🪛 Biome (2.1.2)
src/api/APIFactory.js
[error] 473-473: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 473-473: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/common/types/core.js
[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/features/metadata-based-view/MetadataQueryAPIHelper.js
[error] 57-57: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 59-59: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 59-59: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 163-163: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 163-163: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 171-171: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 171-171: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 171-171: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 174-174: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (21)
src/elements/common/messages.js (1)
1092-1102: LGTM! Well-structured internationalization message.The new
numFilesSelectedmessage follows all established patterns:
- Proper ICU pluralization syntax with all necessary forms (zero, one, other)
- Consistent naming convention with the
be.contentExplorernamespace- Clear description explaining its purpose
- Proper formatting and indentation
src/common/types/core.js (2)
37-37: LGTM! Import addition supports the Collection type enhancement.The
MetadataTemplateimport is correctly added to support the new optional property in theCollectiontype.
397-397: LGTM! Well-designed type enhancement for metadata view V2.The optional
metadataTemplateproperty correctly extends theCollectiontype to support the new metadata view functionality while maintaining backward compatibility.src/api/APIFactory.js (1)
473-476: LGTM! Consistent with established patterns in the codebase.The modification brings
getFolderAPIin line with other API getter methods likegetFileAPI,getMetadataAPI, etc., which already support the optionalshouldDestroyparameter. The default value oftruemaintains backward compatibility.This change enables more flexible API lifecycle management, which aligns with the metadata view V2 requirements where API instances may need to be reused without always destroying previous instances.
src/features/metadata-based-view/MetadataQueryAPIHelper.js (6)
23-23: LGTM! Import addition supports the V2 query field requirements.The
FIELD_EXTENSIONandFIELD_CREATED_ATimports are correctly added to support the enhanced query field verification in metadata view V2.
57-61: LGTM! Clean feature flag implementation.The
isV2constructor parameter with a default value offalseprovides a clean way to enable V2 behavior while maintaining backward compatibility. The implementation follows good practices for feature flag patterns.
155-161: LGTM! Appropriate conditional metadata handling for V2.The conditional logic in
flattenResponseEntrycorrectly preserves the original metadata structure whenisV2is true, while maintaining the existing flattening behavior for backward compatibility.
163-169: LGTM! V2 supports broader item type display.The conditional filtering logic correctly expands V2 to support all item types rather than restricting to files only. This enhances the flexibility of the metadata view while preserving the existing file-only behavior for backward compatibility.
171-181: LGTM! Enhanced data structure supports metadata template access.The addition of
metadataTemplateto the returned collection correctly provides template information to consuming components, aligning with the updatedCollectiontype definition.
247-255: LGTM! V2 query enhancement with additional required fields.The conditional addition of
FIELD_EXTENSIONandFIELD_CREATED_ATensures that metadata view V2 has access to the necessary file information for enhanced functionality, while maintaining the minimal field set for the original implementation.src/elements/common/sub-header/__tests__/SubHeaderLeftMetadataViewV2.test.tsx (4)
1-46: LGTM! Excellent test setup and mocking structure.The test setup demonstrates best practices:
- Proper TypeScript interfaces for test props
- Realistic mock data with appropriate API mocking
- Clean
renderComponenthelper for consistent test rendering- Well-structured default props that can be easily overridden
48-79: LGTM! Comprehensive testing of unselected state behavior.The test cases thoroughly cover the title display logic when no items are selected:
- Proper precedence of custom title over ancestor folder name
- Graceful handling of null/undefined values
- Clear assertions for both positive and negative cases
81-143: LGTM! Thorough testing of selection states and edge cases.The selection tests provide excellent coverage:
- All selection scenarios (single, multiple, all) are tested
- Proper verification of internationalized text rendering
- User interaction testing with close button functionality
- Important edge cases handled (non-existent items, empty collections)
- Robust error condition testing to prevent crashes
145-185: LGTM! Excellent structural and accessibility testing.The final test sections demonstrate comprehensive attention to detail:
- CSS class verification ensures proper styling integration
- Accessibility testing with proper heading role verification
- Complete edge case coverage including zero selections
- UI structure validation for consistent rendering
src/elements/content-explorer/ContentExplorer.tsx (7)
13-13: LGTM! Appropriate import for selection handling.The import of
Selectiontype fromreact-aria-componentsis correctly placed and aligns with the selection management requirements.
177-177: LGTM! State properties are correctly typed and initialized.The new state properties for metadata view v2 are appropriately typed and initialized:
metadataAncestorFolderNameasstring | nullinitialized tonullselectedKeysasSelectioninitialized tonew Set()Also applies to: 182-182, 304-304, 307-307
344-344: LGTM! Lifecycle methods correctly handle metadata ancestor folder fetching.The updates to
componentDidMountandcomponentDidUpdateproperly manage fetching the metadata ancestor folder name:
- On mount when the default view is metadata
- When the
metadataQuery.ancestor_folder_idprop changesAlso applies to: 354-354, 369-389
1618-1620: LGTM! Simple and correct selection clearing method.The method correctly resets the selection state to an empty Set.
1737-1747: LGTM! Well-structured prop merging for selection handling.The implementation correctly:
- Preserves existing
metadataViewPropsandtableProps- Adds selection state management
- Calls the original
onSelectionChangehandler before updating local state
1760-1760: LGTM! Props correctly passed to child components.The new props for metadata view v2 are properly passed to:
SubHeader: receivesapi,selectedKeys,onClearSelectedKeys,metadataAncestorFolderName, andmetadataViewTitleContent: receives the combinedmetadataViewPropswith selection handlingAlso applies to: 1780-1783, 1801-1801
1639-1639: All getFolderAPI(false) usages confirmed
- Only one occurrence of
getFolderAPI(false)exists in
src/elements/content-explorer/ContentExplorer.tsx- No other calls with a boolean argument were found
- No further updates are needed for this flag usage
jfox16
force-pushed
the
subheader-left-metadata-view-v2
branch
from
c1281fb to
015674e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
1628-1649: Consider localizing the hardcoded "All Files" string.The method logic is sound with proper error handling and edge case management. However, the hardcoded "All Files" string should be localized for better i18n support.
Consider extracting the hardcoded string to a localized message:
- this.setState({ metadataAncestorFolderName: 'All Files' }); + this.setState({ metadataAncestorFolderName: messages.allFiles || 'All Files' });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
i18n/en-US.properties(1 hunks)src/elements/common/messages.js(1 hunks)src/elements/common/sub-header/SubHeader.tsx(6 hunks)src/elements/common/sub-header/SubHeaderLeftMetadataViewV2.tsx(1 hunks)src/elements/common/sub-header/__tests__/SubHeaderLeftMetadataViewV2.test.tsx(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- i18n/en-US.properties
- src/elements/common/messages.js
- src/elements/common/sub-header/tests/SubHeaderLeftMetadataViewV2.test.tsx
- src/elements/common/sub-header/SubHeaderLeftMetadataViewV2.tsx
- src/elements/common/sub-header/SubHeader.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ahorowitz123
PR: box/box-ui-elements#4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, `isExistingAIExtractionCascadePolicy` specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: `VersionSidebarView` intentionally uses the `versionId` field to stay consistent with current URL parameter naming; a potential rename to `fileVersionId` is deferred until React Router is removed.
src/elements/content-explorer/ContentExplorer.tsx (4)
Learnt from: ahorowitz123
PR: #4102
File: src/features/metadata-instance-editor/Instance.js:647-649
Timestamp: 2025-05-14T17:46:25.370Z
Learning: In the metadata-instance-editor component, isExistingAIExtractionCascadePolicy specifically checks if the cascade policy fetched from the backend has AI folder extraction enabled, using props rather than state to reflect the original server-side configuration rather than current UI state.
Learnt from: rafalmaksymiuk
PR: #4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Learnt from: jpan-box
PR: #4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Learnt from: rafalmaksymiuk
PR: #4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: VersionSidebarView intentionally uses the versionId field to stay consistent with current URL parameter naming; a potential rename to fileVersionId is deferred until React Router is removed.
⏰ 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 (11)
src/elements/content-explorer/ContentExplorer.tsx (11)
13-14: LGTM! Import for Selection type is appropriate.The import of
Selectiontype fromreact-aria-componentsis correctly added to support the new multi-selection functionality in metadata view.
137-137: LGTM! Optional prop addition is well-designed.The
metadataViewTitleprop is appropriately typed as optional and aligns with the metadata view V2 feature requirements.
177-177: LGTM! State type additions are properly typed.Both
metadataAncestorFolderNameandselectedKeysare appropriately typed with proper null handling and Selection type respectively.Also applies to: 182-182
245-245: LGTM! Default values and state initialization are correct.The default values and state initialization are appropriate:
- Empty string default for
metadataViewTitlenullinitialization formetadataAncestorFolderName(to be fetched)new Set()forselectedKeys(correct for Selection type)Also applies to: 304-304, 307-307
344-344: LGTM! ComponentDidMount changes are logically sound.The addition of
metadataQuerydestructuring and conditional fetching of ancestor folder name only for metadata view is appropriate and uses safe optional chaining.Also applies to: 354-355
369-389: LGTM! ComponentDidUpdate logic correctly handles ancestor folder changes.The update logic properly compares previous and current
ancestor_folder_idvalues and only fetches the folder name when there's an actual change, using safe optional chaining.
1618-1620: LGTM! Selection clearing method is correctly implemented.The
handleClearSelectedKeysmethod properly resets the selection state to an empty Set, which is the correct approach for the Selection type.
1685-1685: LGTM! Props and state destructuring additions are correct.The additions of
metadataViewTitleto props destructuring andmetadataAncestorFolderNameto state destructuring are properly implemented.Also applies to: 1716-1716
1737-1747: LGTM! Selection handling integration is well-implemented.The
combinedMetadataViewPropscorrectly merges existing metadata view props with new selection handling. The selection change handler properly calls existing handlers before updating local state.
1773-1773: LGTM! SubHeader props for metadata view are correctly configured.The props passed to SubHeader correctly implement the fallback logic (
metadataViewTitle || metadataAncestorFolderName) and include the necessary selection-related props (selectedKeys,onClearSelectedKeys).Also applies to: 1781-1782
1800-1800: LGTM! Content component receives properly merged metadata view props.The
combinedMetadataViewPropsis correctly passed to the Content component, ensuring selection handling is integrated throughout the metadata view.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/elements/common/sub-header/SubHeaderLeftV2.tsx (2)
12-18: Remove unusedapiprop from interface.The
apiprop is defined in the interface but never used within the component implementation. Consider removing it to keep the interface clean.export interface SubHeaderLeftV2Props { - api?: API; currentCollection: Collection; title?: string; onClearSelectedKeys?: () => void; selectedKeys: Selection; }
24-46: Remove unnecessary type assertion and consider refactoring for clarity.The logic is sound but has room for improvement:
- Line 38 has an unnecessary type assertion since you've already verified the type
- Consider using early returns for better readability
const selectedItemText = useMemo(() => { const selectedCount = selectedKeys === 'all' ? currentCollection.items.length : selectedKeys.size; if (typeof selectedCount !== 'number' || selectedCount === 0) { return ''; } // Case 1: Single selected item - show item name if (selectedCount === 1) { const selectedKey = selectedKeys === 'all' ? currentCollection.items[0].id : selectedKeys.values().next().value; const selectedItem = currentCollection.items.find(item => item.id === selectedKey); - if (typeof selectedItem?.name === 'string') { - return selectedItem.name as string; - } + return selectedItem?.name || ''; } // Case 2: Multiple selected items - show count - if (selectedCount > 1) { - return formatMessage(messages.numFilesSelected, { numSelected: selectedCount }); - } - return ''; + return formatMessage(messages.numFilesSelected, { numSelected: selectedCount }); }, [currentCollection.items, formatMessage, selectedKeys]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/elements/common/sub-header/SubHeader.scss(1 hunks)src/elements/common/sub-header/SubHeader.tsx(6 hunks)src/elements/common/sub-header/SubHeaderLeftV2.scss(1 hunks)src/elements/common/sub-header/SubHeaderLeftV2.tsx(1 hunks)src/elements/common/sub-header/__tests__/SubHeaderLeftV2.test.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/elements/common/sub-header/SubHeaderLeftV2.scss
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/common/sub-header/SubHeader.scss
- src/elements/common/sub-header/SubHeader.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: `VersionSidebarView` intentionally uses the `versionId` field to stay consistent with current URL parameter naming; a potential rename to `fileVersionId` is deferred until React Router is removed.
⏰ 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 (7)
src/elements/common/sub-header/SubHeaderLeftV2.tsx (2)
20-23: LGTM!Clean component setup with proper destructuring and hook usage.
48-64: LGTM!Clean conditional rendering with proper accessibility considerations. Good use of semantic HTML elements and consistent CSS class naming.
src/elements/common/sub-header/__tests__/SubHeaderLeftV2.test.tsx (5)
1-22: LGTM!Excellent test setup with proper mock data, reusable render helper, and clean organization.
23-33: LGTM!Clean test for the no-selection state with appropriate assertions.
35-97: Excellent test coverage for selection scenarios.These tests comprehensively cover all selection states including edge cases like non-existent items and empty collections. The interaction testing with the close button callback is well implemented.
99-117: LGTM!Good structural tests that verify CSS class integration for proper styling.
119-129: LGTM!Good edge case test that verifies the absence of the close button when no items are selected.
jfox16
force-pushed
the
subheader-left-metadata-view-v2
branch
2 times, most recently
from
3b8a237 to
568efdc
Compare
jfox16
force-pushed
the
subheader-left-metadata-view-v2
branch
from
568efdc to
8a3fcb5
Compare
jfox16
force-pushed
the
subheader-left-metadata-view-v2
branch
from
f0e3c6d to
059463c
Compare
jfox16
force-pushed
the
subheader-left-metadata-view-v2
branch
from
059463c to
e3fdcb1
Compare
…r-left-metadata-view-v2
3 participants
Screen.Recording.2025-07-29.at.11.46.57.AM.mov
Summary by CodeRabbit
New Features
Style
Tests