feat(metadata-view): Add MetadataView V2 #4191
Conversation
WalkthroughThis change introduces a new metadata view feature for the Content Explorer, including a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ContentExplorer
participant MetadataQueryAPIHelper
participant API
participant MetadataViewContainer
User->>ContentExplorer: Initiate metadata view
ContentExplorer->>MetadataQueryAPIHelper: fetchMetadataQueryResults(query, success, error)
MetadataQueryAPIHelper->>API: queryMetadata(query)
API-->>MetadataQueryAPIHelper: metadata query response
MetadataQueryAPIHelper->>API: getTemplateSchemaInfo()
API-->>MetadataQueryAPIHelper: template schema response
MetadataQueryAPIHelper-->>ContentExplorer: success(flattened collection, metadataTemplate)
ContentExplorer->>MetadataViewContainer: Render with collection, metadataTemplate, props
MetadataViewContainer->>User: Display metadata view with filters and columns
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 (
|
ab8ae04 to
e92ba45
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
scripts/jest/jest.config.js (1)
29-29: LGTM! Jest configuration updated for new metadata packages.The addition of
@box/metadata-filter,@box/metadata-view, and@box/typesto thetransformIgnorePatternsis necessary to ensure Jest properly processes these packages during testing.Consider refactoring this long regex pattern into a more maintainable format, such as:
- 'node_modules/(?!(@box/react-virtualized/dist/es|@box/cldr-data|@box/blueprint-web|@box/blueprint-web-assets|@box/metadata-editor|@box/box-ai-content-answers|@box/box-ai-agent-selector|@box/item-icon|@box/combobox-with-api|@box/tree|@box/metadata-filter|@box/metadata-view|@box/types)/)', + `node_modules/(?!(${[ + '@box/react-virtualized/dist/es', + '@box/cldr-data', + '@box/blueprint-web', + '@box/blueprint-web-assets', + '@box/metadata-editor', + '@box/box-ai-content-answers', + '@box/box-ai-agent-selector', + '@box/item-icon', + '@box/combobox-with-api', + '@box/tree', + '@box/metadata-filter', + '@box/metadata-view', + '@box/types' + ].join('|')})/)`src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (3)
83-83: Remove debug statement from production test.The
screen.debug(null, 10000)call should be removed as it's only useful during development and can clutter test output.- screen.debug(null, 10000);
15-32: Fix inconsistency in mock template field data.There's an inconsistency in the mock data: the first field has
key: ' name'with a leading space, which seems unintentional.{ id: 'field1', - key: ' name', + key: 'name', displayName: 'Name', type: 'string', },
81-89: Consider expanding test coverage.While the current test verifies basic rendering, consider adding tests for:
- Sort functionality via onSortChange callback
- Different column configurations
- Empty state handling
- Error states
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
38-45: Consider more flexible column widths.The fixed 150px width for all columns might not accommodate varying content lengths effectively. Consider using minWidth with flexible maxWidth or auto-sizing.
const columns = mockSchema.fields.map(field => ({ textValue: field.displayName, id: `${metadataSourceFieldName}.${field.key}`, type: field.type, allowSorting: true, - minWidth: 150, - maxWidth: 150, + minWidth: 150, + maxWidth: 300, }));src/elements/content-explorer/stories/MetadataView.stories.tsx (3)
17-22: Consider improving the documentation for the commented filter examples.The commented code provides valuable examples for filtering, but could benefit from more descriptive comments explaining when and how to use these filters.
- // // Filter items in the folder by existing metadata key - // query: 'key = :arg1', - // - // // Display items with value - // query_params: { arg1: 'value' }, + // Example: Filter items by metadata field value + // Uncomment below to show only items where a specific metadata field equals a value + // query: 'industry = :industry_value', + // query_params: { industry_value: 'Technology' },
39-50: Add defensive check for name field existence.The code assumes a field with
key === 'name'exists in the schema. Consider adding a defensive check or documenting this requirement.const columns = mockSchema.fields.map(field => { if (field.key === 'name') { return { textValue: field.displayName, id: 'name', type: 'string', allowsSorting: true, minWidth: 250, maxWidth: 250, isRowHeader: true, }; } +}).filter(Boolean); // Filter out any undefined values if name field is missing
60-63: Consider locale-aware date formatting.The date formatting uses
toLocaleDateString()without specifying a locale, which could lead to inconsistent formatting across different user environments.cellRenderer: (item, column) => { const dateValue = get(item, column.id); - return dateValue ? new Date(dateValue).toLocaleDateString() : ''; + return dateValue ? new Date(dateValue).toLocaleDateString('en-US', { + year: 'numeric', + month: 'short', + day: 'numeric' + }) : ''; },
📜 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 (16)
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/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(10 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 (7)
📓 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.
scripts/i18n.config.js (3)
Learnt from: tjuanitas
PR: box/box-ui-elements#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: box/box-ui-elements#4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Learnt from: tjuanitas
PR: box/box-ui-elements#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: box/box-ui-elements#4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Learnt from: tjuanitas
PR: box/box-ui-elements#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/stories/MetadataView.stories.tsx (1)
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.
package.json (3)
Learnt from: jpan-box
PR: box/box-ui-elements#4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#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: tjuanitas
PR: box/box-ui-elements#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 (1)
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.
src/elements/content-explorer/ContentExplorer.tsx (3)
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#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: 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.
🧬 Code Graph Analysis (4)
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(1854-1854)
src/elements/content-explorer/stories/MetadataView.stories.tsx (2)
src/elements/common/__mocks__/mockMetadata.ts (2)
mockSchema(226-226)mockMetadata(226-226)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
metadataView(50-56)
src/elements/content-explorer/Content.tsx (2)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
MetadataViewContainerProps(7-10)src/elements/content-picker/Content.js (1)
currentCollection(47-47)
src/elements/content-explorer/ContentExplorer.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
MetadataViewContainerProps(7-10)
🪛 Biome (1.9.4)
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)
[error] 164-168: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 172-172: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 247-247: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 248-254: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
⏰ 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 (31)
scripts/i18n.config.js (1)
8-9: LGTM! Translation dependencies correctly added.The addition of
@box/metadata-filterand@box/metadata-viewto the translation dependencies aligns with the new metadata view functionality being introduced in this PR.src/common/types/core.js (2)
38-38: LGTM! MetadataTemplate import added correctly.The import statement properly brings in the MetadataTemplate type needed for the Collection type enhancement.
398-398: LGTM! Collection type enhanced with metadata template support.The addition of the optional
metadataTemplateproperty to the Collection type correctly supports the new metadata view functionality while maintaining backward compatibility.src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js (1)
189-189: LGTM! Test fixture updated to match new API behavior.The addition of
metadataTemplate: templateto theflattenedDataWithTypestest fixture correctly reflects the updated behavior where thegetFlattenedDataWithTypesmethod now includes the metadata template in the returned collection object.src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
413-413: Test assertion updated correctly.The test assertion has been appropriately updated to check for the specific file name that matches the new mock data structure and V2 implementation behavior.
package.json (1)
127-140: Confirm package versions; run local security audit
- File: package.json (lines 127–140)
- All specified versions exist on the npm registry—no missing releases detected.
- Security advisories can’t be fully assessed in this sandbox. Please run
npm audit(ornpm audit --production) in your local environment to ensure no moderate-or-higher vulnerabilities were introduced by these updates.src/elements/content-explorer/__tests__/Content.test.tsx (3)
40-43: LGTM: Appropriate mocking strategy for unit tests.The mock implementation correctly replaces the MetadataViewContainer with a simple identifiable element for testing purposes.
96-102: Good refactor: Extracting shared test data.Extracting the collection object reduces duplication and improves test maintainability.
123-123: Test assertion correctly updated for new component.The test now correctly expects the mocked "MetadataViewContainer" text instead of the previous component's output.
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (3)
1-1: Good practice: Explicit React import.Adding the explicit React import follows modern React best practices and ensures compatibility.
60-62: LGTM: Proper integration of metadata props.The metadataProps structure correctly passes the columns configuration to the new MetadataViewContainer component.
72-78: Good enhancement: Custom render wrapper.The padding wrapper improves the visual presentation of the story in Storybook.
src/elements/common/__mocks__/mockMetadata.ts (3)
4-4: LGTM: Addition of extension fields for enhanced metadata support.The explicit
extensionfields added to file entries align with the enhanced metadata view requirements and provide better file type identification.Also applies to: 26-26, 48-48, 69-69, 90-90
20-21: Appropriate timestamp field changes.The change from
modified_attocreated_ataligns with the updated metadata schema and API expectations for the V2 implementation.Also applies to: 42-43, 62-64, 83-85, 102-104
108-144: Excellent addition: Folder entries with metadata.Adding folder entries with metadata supports testing of the expanded item type support mentioned in the PR objectives, moving beyond files-only to all item types.
src/elements/content-explorer/Content.tsx (4)
7-7: Import change looks good.The import correctly brings in the new
MetadataViewContainercomponent and its props type, aligning with the architectural change to use the wrapper component.
39-39: Well-designed prop type definition.The use of
Omit<MetadataViewContainerProps, 'currentCollection'>is appropriate here, ascurrentCollectionis already available in the Content component's props and shouldn't be passed throughmetadataProps.
57-57: Props destructuring is correct.The
metadataPropsis properly added to the destructured props list.
84-92: Verify empty‐state support in MetadataViewThe new conditional drops the
!isViewEmptyguard, soMetadataViewContainerwill always render<MetadataView … items={[]} />when the collection is empty. We weren’t able to find any empty-state logic inMetadataViewContaineritself, nor can we inspect the external@box/metadata-viewpackage here.Please ensure that
<MetadataView>:
- Renders an appropriate empty state (e.g. “No items” message or placeholder) when its
itemsprop is an empty array- Or, if it does not handle empty collections internally, consider re-adding the
!isViewEmptycheck before renderingsrc/elements/content-explorer/stories/MetadataView.stories.tsx (1)
80-104: Story configuration is well-structured.The story properly configures the metadata view with columns, enables the V2 feature flag, and provides a clear visual container for the component demonstration.
src/features/metadata-based-view/MetadataQueryAPIHelper.js (5)
23-23: Constructor and import changes are correct.The addition of
FIELD_EXTENSIONandFIELD_CREATED_ATimports and theisV2flag with a default value maintains backward compatibility while enabling the new metadata view features.Also applies to: 57-62
155-160: Conditional metadata handling is appropriate.The V2 mode correctly returns raw metadata without flattening, which aligns with the new metadata view's capability to handle the original metadata structure.
163-168: Expanded item type support for V2.The V2 mode correctly returns all item types (files, folders, web links) instead of filtering to files only, enabling the new metadata view to display various content types.
171-180: Metadata template inclusion is necessary.Adding
metadataTemplateto the returned collection is required for theMetadataViewContainerto configure filters based on the template fields.
247-256: Additional required fields for V2 are appropriate.The V2 mode correctly requires
extensionandcreated_atfields in addition toname, providing more comprehensive file information for the enhanced metadata view.src/elements/content-explorer/MetadataViewContainer.tsx (3)
7-10: Well-designed interface.The interface correctly extends
MetadataViewPropswhile omittingitemsto avoid conflicts, as items are extracted fromcurrentCollection.
41-46: Sort handler implementation is correct.The callback properly handles the sort descriptor and forwards it to the parent component with appropriate type casting.
48-63: Component composition is well-structured.The component correctly composes props with proper precedence - spreading incoming props for
actionBarPropsandtablePropswhile adding the computedfilterGroupsandhandleSortChange.src/elements/content-explorer/ContentExplorer.tsx (3)
176-176: Type extension for flexible sorting.The
sortBy: SortBy | stringtype change appropriately supports sorting by metadata fields while maintaining compatibility with existing sort options.
403-442: Metadata query enhancements are well-implemented.The method correctly:
- Preserves existing
order_byvalues while adding new sort parameters- Conditionally instantiates
MetadataQueryAPIHelperwith V2 mode based on feature flag- Maintains backward compatibility with existing queries
854-863: Sort method correctly supports metadata view.The condition
if (id || view === VIEW_METADATA)appropriately enables sorting for both folder views (with id) and metadata views.
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx
Outdated
Show resolved
Hide resolved
3c9bd8c to
b49985c
Compare
| const filterGroups = React.useMemo( | ||
| () => [ | ||
| { | ||
| togglable: true, |
There was a problem hiding this comment.
toggleable vs togglable
If just fixing this causes you to have to fight e2e tests 5 times in a row, I wouldn't mind leaving this alone.
There was a problem hiding this comment.
it's written like that in the shared-feature 🙈
| @@ -54,8 +54,11 @@ export default class MetadataQueryAPIHelper { | |||
|
|
|||
| metadataQuery: MetadataQueryType; | |||
|
|
|||
| constructor(api: API) { | |||
| isV2: boolean; | |||
There was a problem hiding this comment.
@coderabbitai how is isV2 set by a consumer of the Metadata View element?
There was a problem hiding this comment.
🧩 Analysis chain
I need to trace how the isV2 parameter flows from consumers to the MetadataQueryAPIHelper. Let me examine the code to understand this flow.
@jpan-box Based on my analysis of the codebase, here's how isV2 is set by consumers of the Metadata View element:
Flow from Consumer to MetadataQueryAPIHelper
-
Consumer Level: Consumers pass a
featuresprop to theContentExplorercomponent containing feature flag configurations. -
ContentExplorer Level: In
showMetadataQueryResults(), the ContentExplorer checks if thecontentExplorer.metadataViewV2feature flag is enabled usingisFeatureEnabled(features, 'contentExplorer.metadataViewV2'). -
MetadataQueryAPIHelper Construction: The result of this feature flag check is passed as the second parameter to the MetadataQueryAPIHelper constructor:
new MetadataQueryAPIHelper(this.api, isFeatureEnabled(features, 'contentExplorer.metadataViewV2'))
Consumer Usage
Consumers enable the V2 metadata view by including the feature flag in their features prop:
<ContentExplorer
features={{
'contentExplorer.metadataViewV2': true
}}
// other props...
/>When this feature flag is enabled, the MetadataQueryAPIHelper operates in V2 mode, which:
- Returns raw metadata without flattening it
- Includes all entry types (not just files)
- Adds
FIELD_EXTENSIONandFIELD_CREATED_ATto required fields - Includes the
metadataTemplatein the returned collection
So consumers control the isV2 behavior through feature flags rather than setting it directly.
🏁 Script executed:
#!/bin/bash
# Find how MetadataQueryAPIHelper is instantiated with the isV2 parameter
rg "new MetadataQueryAPIHelper" -A 2 -B 2
Length of output: 1051
🏁 Script executed:
#!/bin/bash
# Find how the metadataViewV2 feature flag is used
rg "metadataViewV2" -A 3 -B 3
Length of output: 6815
🏁 Script executed:
#!/bin/bash
# Look for feature flag usage in ContentExplorer
rg "isFeatureEnabled.*contentExplorer" -A 2 -B 2
Length of output: 1050
🏁 Script executed:
#!/bin/bash
# Find examples of how consumers pass features prop
rg "features.*=|features:" -A 2 -B 2
Length of output: 264946
@jpan-box Consumers never touch isV2 directly.
They enable it by passing the contentExplorer.metadataViewV2 feature flag to ContentExplorer, e.g.
<ContentExplorer
features={{ contentExplorer: { metadataViewV2: true } }}
…otherProps
/>ContentExplorer checks that flag with isFeatureEnabled and passes the result as the second argument to
new MetadataQueryAPIHelper(this.api, /* isV2 */ true);So turning the flag on (or off) in the features prop is what sets isV2.
🧬 Code Graph Analysis Results
Relevant snippet from src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js lines 195-207:
const api = {
getMetadataAPI: () => {
return {
getSchemaByTemplateKey: getSchemaByTemplateKeyFunc,
updateMetadata: updateMetadataFunc,
};
},
getMetadataQueryAPI: () => {
return {
queryMetadata: queryMetadataFunc,
};
},
};This snippet shows a mock API object used in tests for MetadataQueryAPIHelper. It includes methods getMetadataAPI() and getMetadataQueryAPI(), which are used by MetadataQueryAPIHelper instances. This is relevant because the constructor of MetadataQueryAPIHelper takes an api parameter, and the isV2 flag is passed alongside it. Understanding how the API is mocked and used in tests can help clarify how isV2 might be set or used by consumers.
Relevant snippet from src/features/metadata-based-view/MetadataBasedItemList.js lines 122 and 251:
// line 122
const fields = getProp(this.props, 'currentCollection.items[0].metadata.enterprise.fields', []);
// line 251
const fields = getProp(item, 'metadata.enterprise.fields', []);These lines show access to metadata fields in the enterprise scope, which is consistent with how MetadataQueryAPIHelper flattens metadata under enterprise. This is relevant context for understanding how metadata is consumed and possibly how the isV2 flag might affect data structure or usage.
Relevant snippet from src/features/metadata-based-view/MetadataBasedItemList.js lines 357-358:
const displayName =
getProp(field, 'displayName') || getProp(find(responseFields, ['key', key]), 'displayName', key);This snippet shows how display names for metadata fields are resolved, which relates to how metadata fields are processed and displayed. This could be relevant for understanding differences in behavior when isV2 is true or false.
No direct snippet explicitly shows how isV2 is set by a consumer of the Metadata View element, but the test mock API and usage patterns in MetadataBasedItemList.js provide relevant context for understanding the environment in which MetadataQueryAPIHelper and its isV2 flag operate.
🧠 Learnings used
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.
There was a problem hiding this comment.
can we give a better naming for isV2? for example isMetadataViewV2
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -88,6 +93,7 @@ import '../common/fonts.scss'; | |||
| import '../common/base.scss'; | |||
| import '../common/modal.scss'; | |||
| import './ContentExplorer.scss'; | |||
| import { MetadataViewContainerProps } from './MetadataViewContainer'; | |||
There was a problem hiding this comment.
this should have type import or it might cause issues for consumers
| @@ -124,6 +130,7 @@ export interface ContentExplorerProps { | |||
| measureRef?: (ref: Element | null) => void; | |||
| messages?: StringMap; | |||
| metadataQuery?: MetadataQuery; | |||
| metadataProps?: Omit<MetadataViewContainerProps, 'hasError' | 'currentCollection'>; | |||
There was a problem hiding this comment.
we should have an external facing type similar to contentUploaderProps or contentPreviewProps for consumers. this type doesn't match Content.tsx
There was a problem hiding this comment.
what would you want to rename to this to ? contentMetadataProps?
There was a problem hiding this comment.
we can create a new file and move it to the src/elements folder to handle v2 instead of updating this one in src/features
| if (!clonedFields.includes(FIELD_CREATED_AT)) { | ||
| clonedFields.push(FIELD_CREATED_AT); | ||
| } |
There was a problem hiding this comment.
grid tiles needs the created at from the item
There was a problem hiding this comment.
unless im supposed to put the modified at there???
| const filterGroups = React.useMemo( | ||
| () => [ | ||
| { | ||
| togglable: true, |
There was a problem hiding this comment.
it's written like that in the shared-feature 🙈
| filters: | ||
| metadataTemplate?.fields?.map(field => { | ||
| return { | ||
| id: `${field.key}-filter`, | ||
| name: field.displayName, | ||
| fieldType: field.type, | ||
| options: field.options?.map(({ key }) => key) || [], | ||
| shouldRenderChip: true, | ||
| }; | ||
| }) || [], |
There was a problem hiding this comment.
isn't filters supposed to be configurable by the consumer?
There was a problem hiding this comment.
separate ticket to handle this filter logic, this is a placeholder so that the filter chips are rendering
| columns={columns} | ||
| items={items} | ||
| actionBarProps={{ |
| actionBarProps={{ | ||
| ...actionBarProps, | ||
| filterGroups, | ||
| }} | ||
| tableProps={{ | ||
| ...tableProps, | ||
| onSortChange: handleSortChange, | ||
| }} |
There was a problem hiding this comment.
what if the consumer wants to control these?
| actionBarProps={{ | |
| ...actionBarProps, | |
| filterGroups, | |
| }} | |
| tableProps={{ | |
| ...tableProps, | |
| onSortChange: handleSortChange, | |
| }} | |
| actionBarProps={{ | |
| filterGroups, | |
| ...actionBarProps, | |
| }} | |
| tableProps={{ | |
| onSortChange: handleSortChange, | |
| ...tableProps, | |
| }} |
There was a problem hiding this comment.
dont know what i was doing with handleSortChange but i removed it and it will just get passed all the way through, filterGroups though that is not set by the consumer, its derived by metadataTemplate which is called automatically via /schema endpoint
| return { | ||
| items: entries.map<BoxItem>(this.flattenResponseEntry), | ||
| metadataTemplate: this.metadataTemplate, |
There was a problem hiding this comment.
feels weird to include this property in the return of getFlattenedDataWithTypes
b49985c to
8a8a0ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/common/types/core.js (1)
397-397: Consider using a specialized Collection type for metadata viewsBuilding on the previous discussion, adding
metadataTemplateto the coreCollectiontype does feel like mixing concerns. While CodeRabbit's suggestion of specialized Collection types (e.g.,MetadataCollection) would be architecturally cleaner, I understand the timeline constraints mentioned.For now, this optional property maintains backward compatibility. However, I'd recommend tracking this as technical debt for future refactoring when time permits.
🧹 Nitpick comments (1)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
177-191: Consider adding error handling for malformed metadataThe method assumes metadata follows the expected structure. Consider adding defensive checks for edge cases.
getTemplateSchemaInfo = (data: MetadataQueryResponseData): Promise<MetadataTemplateSchemaResponse | void> => { const { entries } = data; this.metadataQueryResponseData = this.filterMetdataQueryResponse(data); if (!entries || entries.length === 0) { // Don't make metadata API call to get template info return Promise.resolve(); } const metadata = getProp(entries, '[0].metadata'); + if (!metadata || typeof metadata !== 'object') { + return Promise.reject(new Error('Invalid metadata structure in query response')); + } + this.templateScope = Object.keys(metadata)[0]; + if (!this.templateScope) { + return Promise.reject(new Error('No template scope found in metadata')); + } + const instance = metadata[this.templateScope]; + if (!instance || typeof instance !== 'object') { + return Promise.reject(new Error('Invalid metadata instance structure')); + } + this.templateKey = Object.keys(instance)[0]; + if (!this.templateKey) { + return Promise.reject(new Error('No template key found in metadata instance')); + } return this.api.getMetadataAPI(true).getSchemaByTemplateKey(this.templateKey); };
📜 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 (16)
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/content-explorer/Content.tsx(5 hunks)src/elements/content-explorer/ContentExplorer.tsx(16 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(1 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__/MetadataQueryAPIHelper.test.ts(1 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)
💤 Files with no reviewable changes (1)
- src/elements/content-explorer/MetadataView.tsx
🚧 Files skipped from review as they are similar to previous changes (12)
- scripts/i18n.config.js
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
- scripts/jest/jest.config.js
- src/elements/content-explorer/tests/MetadataViewContainer.test.tsx
- src/elements/common/mocks/mockMetadata.ts
- src/elements/content-explorer/stories/MetadataView.stories.tsx
- src/elements/content-explorer/tests/Content.test.tsx
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
- package.json
- src/elements/content-explorer/MetadataViewContainer.tsx
- src/elements/content-explorer/Content.tsx
- src/elements/content-explorer/ContentExplorer.tsx
🧰 Additional context used
🧠 Learnings (1)
src/common/types/core.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.
🪛 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)
⏰ 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 (1)
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
1-476: Well-structured and comprehensive test coverage!The test suite thoroughly covers all methods of MetadataQueryAPIHelper with appropriate edge cases, mocking patterns, and async handling. Great job on the test organization and coverage.
| }; | ||
| }; | ||
|
|
||
| filterMetdataQueryResponse = (response: MetadataQueryResponseData): MetadataQueryResponseData => { |
There was a problem hiding this comment.
Fix typo in method name
The method name has a typo: filterMetdataQueryResponse should be filterMetadataQueryResponse.
- filterMetdataQueryResponse = (response: MetadataQueryResponseData): MetadataQueryResponseData => {
+ filterMetadataQueryResponse = (response: MetadataQueryResponseData): MetadataQueryResponseData => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filterMetdataQueryResponse = (response: MetadataQueryResponseData): MetadataQueryResponseData => { | |
| filterMetadataQueryResponse = (response: MetadataQueryResponseData): MetadataQueryResponseData => { |
🤖 Prompt for AI Agents
In src/elements/content-explorer/MetadataQueryAPIHelper.ts at line 158, rename
the method from filterMetdataQueryResponse to filterMetadataQueryResponse to
correct the typo in the method name.
|
|
||
| fetchMetadataQueryResults = ( | ||
| metadataQuery: MetadataQueryType, | ||
| successsCallback: SuccessCallback, |
There was a problem hiding this comment.
Fix typo in parameter name
The parameter name has a typo with an extra 's'.
- successsCallback: SuccessCallback,
+ successCallback: SuccessCallback,Also update the reference on line 209:
- return successsCallback(collection, this.metadataTemplate);
+ return successCallback(collection, this.metadataTemplate);🤖 Prompt for AI Agents
In src/elements/content-explorer/MetadataQueryAPIHelper.ts at line 201, correct
the typo in the parameter name from "successsCallback" to "successCallback".
Also update all references to this parameter, including the one on line 209, to
use the corrected name.
| field: string, | ||
| oldValue: MetadataFieldValue | null, | ||
| newValue: MetadataFieldValue | null, | ||
| successsCallback: () => void, |
There was a problem hiding this comment.
Fix typo in parameter name
Same typo as in fetchMetadataQueryResults - parameter has an extra 's'.
- successsCallback: () => void,
+ successCallback: () => void,Also update the reference on line 225:
- .updateMetadata(file, this.metadataTemplate, operations, successsCallback, errorCallback);
+ .updateMetadata(file, this.metadataTemplate, operations, successCallback, errorCallback);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| successsCallback: () => void, | |
| // … within MetadataQueryAPIHelper.ts … | |
| public updateMetadata( | |
| file: File, | |
| metadataTemplate: MetadataTemplate, | |
| operations: JSONPatchOperation[], | |
| successCallback: () => void, | |
| errorCallback: (error: any) => void | |
| ) { | |
| // build JSON-Patch operations… | |
| return this.apiClient | |
| .updateMetadata( | |
| file, | |
| this.metadataTemplate, | |
| operations, | |
| successCallback, | |
| errorCallback | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/elements/content-explorer/MetadataQueryAPIHelper.ts at line 219, correct
the typo in the parameter name from "successsCallback" to "successCallback".
Also, update all references to this parameter, including the one on line 225, to
use the corrected name "successCallback".
MetadataViewmetadataPropsto provides props to theMetadataViewcomponent directlyMetadataQueryAPIHelperto switch to new logic for the v2 design such as retrieving extensions and created at date. Also, allows all item types to return instead of files onlyallowsSortingprop in thecolumndataSummary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation