feat(metadata-view): Multi value display and onSave in metadata sidepanel #4245
Conversation
WalkthroughI pity the fool who don't read — Adds i18n strings, folder-aware metadata API and bulk-patch support, a V2 metadata update flow across ContentExplorer/MetadataQueryAPIHelper/MetadataSidePanel, multi-select template handling utilities, notification integration, tests, and stories. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant MSP as MetadataSidePanel
participant CE as ContentExplorer
participant MQH as MetadataQueryAPIHelper
participant API as Metadata API
participant Notif as Notification
Note over MSP,CE: User saves metadata edits (single or multi)
User->>MSP: Click "Save"
MSP->>MSP: Build templateNewFields from form
MSP->>MSP: Get templateOldFields via useTemplateInstance
alt Multiple items
loop For each item
MSP->>CE: request updateMetadataV2 (ops/template fields)
CE->>MQH: generateOperations / updateMetadataWithOperations
MQH->>API: PATCH metadata (routes by item.type)
API-->>MQH: Response
MQH-->>CE: Result
end
else Single item
MSP->>CE: request updateMetadataV2 (ops/template fields)
CE->>MQH: generateOperations / updateMetadataWithOperations
MQH->>API: PATCH metadata (routes by item.type)
API-->>MQH: Response
MQH-->>CE: Result
end
alt Success
MSP->>Notif: Show success notification (pluralized)
MSP->>CE: refreshCollection()
else Error
MSP->>Notif: Show error notification
end
sequenceDiagram
autonumber
participant CE as ContentExplorer
participant MQH as MetadataQueryAPIHelper
participant API as Metadata API
Note over MQH,API: Metadata patch routing by item.type
CE->>MQH: updateMetadataWithOperations(item, ops)
alt item.type == "file"
MQH->>API: PATCH getMetadataUrl(getTypedFileId(id))
else item.type == "folder"
MQH->>API: PATCH getMetadataUrlForFolder(getTypedFolderId(id))
end
API-->>MQH: Response
MQH-->>CE: Resolve / reject
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.6)src/api/__tests__/Metadata.test.js✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/elements/content-explorer/MetadataSidePanel.tsx (2)
58-64: Guard template instance for empty selections to avoid crashes and improve UXIf the side panel is opened with no items selected, useTemplateInstance will attempt to read selectedItems[0].metadata. Either prevent rendering or provide a placeholder.
Minimal defensive change in this component:
- const templateInstance = useTemplateInstance(metadataTemplate, selectedItems, isEditing); + if (selectedItems.length === 0) { + return null; // or render a lightweight "Select items to view metadata" placeholder + } + const templateInstance = useTemplateInstance(metadataTemplate, selectedItems, isEditing);This also complements the state logic that closes the panel when selection becomes empty.
147-159: Avoid passing null handlers; use undefinedPassing null can break consumers that invoke props without null-guards. Use undefined to omit handlers.
Apply this diff:
- onChange={null} + onChange={undefined}
🧹 Nitpick comments (27)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (2)
205-212: Replace waitFor+getByRole with findByRole for stabilityUse findByRole to both await and select the first row; this removes the extra waitFor wrapper and reduces flakiness. Also aligns with Storybook Testing Library best practices.
- await waitFor(() => { - expect(canvas.getByRole('row', { name: /Child 2/i })).toBeInTheDocument(); - }); - - // Select the first row by clicking its checkbox - const firstItem = canvas.getByRole('row', { name: /Child 2/i }); + // Select the first row by clicking its checkbox + const firstItem = await canvas.findByRole('row', { name: /Child 2/i }); const checkbox = within(firstItem).getByRole('checkbox'); await userEvent.click(checkbox);
215-221: Avoid indexing into getAllByRole; use findByRole and assert checked stateIndexing into getAllByRole can be brittle if the DOM structure changes. Use findByRole to await the row and add explicit assertions that both checkboxes are checked before opening the panel. This hardens the story against intermittent timing issues.
- // Select the second row by clicking its checkbox - const secondItem = canvas.getAllByRole('row', { name: /Child 1/i })[0]; - const secondCheckbox = within(secondItem).getByRole('checkbox'); - await userEvent.click(secondCheckbox); + // Select the second row by clicking its checkbox + const secondItem = await canvas.findByRole('row', { name: /Child 1/i }); + const secondCheckbox = within(secondItem).getByRole('checkbox'); + await userEvent.click(secondCheckbox); + + // Sanity check both are selected + expect(checkbox).toBeChecked(); + expect(secondCheckbox).toBeChecked();i18n/en-US.properties (2)
454-454: Add terminal punctuation for consistencyMost sentences in this file end with a period. Add one here for consistency and polish.
-be.errorNotification = Unable to save changes. Please try again +be.errorNotification = Unable to save changes. Please try again.
844-846: Use generic “item(s)” instead of “document(s)”Metadata edits can target files and folders; “document(s)” may be misleading. Recommend generic “item(s)”.
-be.successNotification = {numSelected, plural, =1 {1 document updated} other {# documents updated} } +be.successNotification = {numSelected, plural, =1 {1 item updated} other {# items updated} }src/elements/common/messages.js (3)
1108-1112: Tighten description grammarMinor grammar cleanup for clarity in the message description.
- multipleValues: { - id: 'be.multipleValues', - description: 'Display text for field when there are multiple items selected and have different value', - defaultMessage: 'Multiple Values', - }, + multipleValues: { + id: 'be.multipleValues', + description: 'Display text for a field when multiple items are selected and have different values', + defaultMessage: 'Multiple Values', + },
1129-1132: Add terminal period to match en-US.properties and house styleKeep punctuation consistent across sources.
- errorNotification: { - id: 'be.errorNotification', - description: 'Text shown in error notification banner', - defaultMessage: 'Unable to save changes. Please try again', - }, + errorNotification: { + id: 'be.errorNotification', + description: 'Text shown in error notification banner', + defaultMessage: 'Unable to save changes. Please try again.', + },
1134-1142: Prefer “item(s)” over “document(s)” for broader applicabilityAligns copy with support for files and folders in the metadata flow.
- successNotification: { - id: 'be.successNotification', - description: 'Text shown in success notification banner', - defaultMessage: ` - {numSelected, plural, - =1 {1 document updated} - other {# documents updated} - } - `, - }, + successNotification: { + id: 'be.successNotification', + description: 'Text shown in success notification banner', + defaultMessage: ` + {numSelected, plural, + =1 {1 item updated} + other {# items updated} + } + `, + },src/api/__tests__/Metadata.test.js (1)
1703-1704: Good alignment with item-based API (explicittype: 'file'). Add coverage for folders.Adding
type: 'file'makes the tests exercise the file path inupdateMetadata. To fully validate the new item-based routing, please add a companion test that covers the folder code path (getMetadataUrlForFolder+getTypedFolderId) and asserts the right URL/id are used.Example test to add:
test('updateMetadata routes to folder endpoint and uses folder-typed id', async () => { const folder = { id: '123', type: 'folder', permissions: { can_upload: true } }; const template = { scope: 'scope', templateKey: 'templateKey' }; const ops = [{ op: 'add', path: '/foo', value: 'bar' }]; metadata.getMetadataUrlForFolder = jest.fn().mockReturnValueOnce('folder_url'); metadata.xhr.put = jest.fn().mockResolvedValueOnce({ data: { $id: 'instance' } }); metadata.isDestroyed = jest.fn().mockReturnValueOnce(false); metadata.getCache = jest.fn().mockReturnValueOnce({ get: () => undefined }); metadata.successHandler = jest.fn(); await metadata.updateMetadata(folder, template, ops, jest.fn(), jest.fn()); expect(metadata.getMetadataUrlForFolder).toHaveBeenCalledWith(folder.id, 'scope', 'templateKey'); expect(metadata.xhr.put).toHaveBeenCalledWith( expect.objectContaining({ url: 'folder_url', id: 'folder_123', data: ops }) ); expect(metadata.successHandler).toHaveBeenCalled(); });Also applies to: 1771-1772, 1838-1839
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (5)
429-439: Clarify test comments and assert content ordering explicitly.
- Nit: The in-line comment says "permission"; the constant is
FIELD_PERMISSIONS. Rename for consistency with the public constant.- Consider asserting the final array equality including order to catch inadvertent reordering later (you currently rely on a spread which can mask ordering issues).
- // Verify "name", "extension" and "permission" are added to pre-existing fields + // Verify "name", "extension" and "permissions" are added to pre-existing fields (order preserved)
441-448: Same nit as above: pluralize "permissions" and assert ordering.- // Verify "extension" and "permission" are added when "name" exists but "extension" and "permission" don't + // Verify "extension" and "permissions" are added when "name" exists but "extension" and "permissions" don't
450-453: Also pluralize "permissions" here.- // Verify "permission" is added + // Verify "permissions" is added
331-341: Add coverage for no-op and array-equality scenarios in JSON Patch creation.The helper now returns [] when "no change" is detected. Please add tests for:
- oldValue === newValue (primitives)
- both empty arrays
- same array contents with different order (e.g., ['a','b'] vs ['b','a'])
- duplicate-sensitive arrays (ensure duplicates are handled or explicitly asserted as unsupported)
Here’s a sample to extend the table:
test.each` oldValue | newValue | ops ${['a', 'b']} | ${['b', 'a']} | ${[]} // reorder only ${[]} | ${[]} | ${[]} // both empty ${'same'} | ${'same'} | ${[]} // identical primitive `('no-op equality cases', ({ oldValue, newValue, ops }) => { expect(metadataQueryAPIHelper.createJSONPatchOperations('field', oldValue as any, newValue as any)).toEqual(ops); });
350-377: Add a unit test forupdateMetadataWithOperations(new public method).To guard the new surface, add a simple test that calls
updateMetadataWithOperationswith a pre-baked operations array and asserts delegation toMetadataAPI.updateMetadata. Also add a no-op case (empty operations) once the guard suggested in the helper is implemented.src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
206-218: Mirror the no-op guard inupdateMetadataas well.
createJSONPatchOperationscan now return[](no-op). Add the same early-return as above to avoid a PUT with empty ops.Proposed change (outside the edited hunk):
const operations = this.createJSONPatchOperations(field, oldValue, newValue); if (!operations.length) { successCallback?.(); return Promise.resolve(); }src/api/Metadata.js (3)
108-121: Docstring nits: method is folder-specific; fix param/return descriptions.The JSDoc mentions "files" for a folder-specific URL helper.
Apply this diff:
- /** - * API URL for metadata - * - * @param {string} id - a Box folder id - * @param {string} field - metadata field - * @return {string} base url for files - */ + /** + * API URL for folder metadata + * + * @param {string} id - a Box folder id + * @param {string} [scope] - metadata scope + * @param {string} [template] - metadata template key + * @return {string} base url for folders + */
839-847: Prefer usingTYPE_FILEconstant and handle unexpecteditem.typedefensively.Using the shared constant avoids magic strings and makes feature-flagged refactors safer. Also consider a sane fallback (file) with a warning when
item.typeis unknown.Apply this diff:
- url: - item.type === 'file' + url: + item.type === TYPE_FILE ? this.getMetadataUrl(id, template.scope, template.templateKey) : this.getMetadataUrlForFolder(id, template.scope, template.templateKey), ... - id: item.type === 'file' ? getTypedFileId(id) : getTypedFolderId(id), + id: item.type === TYPE_FILE ? getTypedFileId(id) : getTypedFolderId(id),Optionally, add:
/* else-if item.type !== 'folder', log a warning or default to file path */
813-819: API surface rename: consider updating parameter JSDoc/comments to say "item (file/folder)".The code already reflects this; bring the surrounding doc comments fully in sync for future readers.
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (7)
149-166: Single-item submit path: assertion shape looks right. Consider asserting no extraneous calls.You can strengthen this by asserting
getOperationswas not invoked in the single-selection path (if that matches the component contract) and that the operations argument is an array (already asserted viaexpect.any(Array)).expect(defaultProps.getOperations).not.toHaveBeenCalled();
167-187: Multi-item path exercised well. Add argument assertions to catch wiring regressions.Also assert that
getOperationsis called with each selected item and two arrays (old/new fields), e.g., usingtoHaveBeenNthCalledWithto ensure correct call ordering and payload.expect(defaultProps.getOperations).toHaveBeenNthCalledWith( 1, mockCollection.items[0], expect.any(Array), expect.any(Array), ); expect(defaultProps.getOperations).toHaveBeenNthCalledWith( 2, mockCollection.items[1], expect.any(Array), expect.any(Array), );
189-212: Success UX: also add a pluralized case with multi-select to lock messaging and view-mode reset.Add a test where two items succeed and assert e.g., "2 documents updated",
refreshCollectioncalled once, and editor returns to view mode.
214-232: Error UX for single-item: good. Consider asserting norefreshCollection.// With a spy on refreshCollection expect(defaultProps.refreshCollection).not.toHaveBeenCalled();
233-252: Error UX for multi-item: good. Consider asserting partial failures don’t leave edit mode.If the implementation keeps edit mode on failure, assert the presence of the Save button to prevent regressions.
254-258: "All" selection subtitle: good. Add a control wheretotalCountdiffers fromitems.length.If the UI uses a server-side "all" selection, ensure the subtitle uses the right count source.
260-293: "Multiple Values" rendering: consider adding a follow-up test that after editing, divergent values are replaced by the new value.This will validate the normalization of multi-selection edits.
src/elements/content-explorer/utils.ts (2)
65-71: Use isEmptyValue for consistency (0 should not be treated as empty)The truthy check will treat 0 as empty/falsey (even though your comment says 0 is not empty) and skip the early return. Using isEmptyValue is clearer and future-proof.
Apply this diff:
function setFieldValue(fieldType: MetadataFieldType, fieldValue: ItemMetadataFieldValue) { - if (fieldValue) return fieldValue; + if (!isEmptyValue(fieldValue)) return fieldValue; if (fieldType === 'multiSelect') return []; if (fieldType === 'enum') return ''; return fieldValue; }
57-60: Potential O(n^2) multiSelect comparison; use Set for linear-time checkFor long lists, includes per element is quadratic. A Set yields linear-time equality and is order-insensitive.
Apply this diff:
- if (fieldType === 'multiSelect' && Array.isArray(value1) && Array.isArray(value2)) { - return value1.length === value2.length && value1.every(val => value2.includes(val)); - } + if (fieldType === 'multiSelect' && Array.isArray(value1) && Array.isArray(value2)) { + if (value1.length !== value2.length) return false; + const s2 = new Set(value2); + return value1.every(val => s2.has(val)); + }src/elements/content-explorer/ContentExplorer.tsx (1)
547-559: Return the promise directly and let callers decide await/thenMinor: Returning the promise aids composability and testing; await is fine but unnecessary here.
Apply this diff:
- ) => { - await this.metadataQueryAPIHelper.updateMetadataWithOperations( + ) => { + return this.metadataQueryAPIHelper.updateMetadataWithOperations( item, operations, successCallback, errorCallback, ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
i18n/en-US.properties(4 hunks)src/api/Metadata.js(5 hunks)src/api/__tests__/Metadata.test.js(3 hunks)src/elements/common/messages.js(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(5 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(5 hunks)src/elements/content-explorer/MetadataSidePanel.tsx(5 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(2 hunks)src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx(3 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)src/elements/content-explorer/utils.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T16:19:21.964Z
Learnt from: greg-in-a-box
PR: box/box-ui-elements#4235
File: src/elements/content-explorer/MetadataQueryBuilder.ts:103-110
Timestamp: 2025-08-25T16:19:21.964Z
Learning: In src/elements/content-explorer/MetadataQueryBuilder.ts, the getSelectFilter function has a special case that maps 'mimetype-filter' to 'item.extension' because mimetype-filter is a multi-select field but the actual database field is 'item.extension'. This mapping is necessary and should not be removed.
Applied to files:
src/elements/content-explorer/utils.ts
🧬 Code graph analysis (5)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
src/elements/content-explorer/utils.ts (1)
isEmptyValue(44-47)
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (2)
src/elements/content-explorer/MetadataSidePanel.tsx (1)
MetadataSidePanelProps(25-42)src/elements/content-explorer/ContentExplorer.tsx (1)
render(1767-2044)
src/elements/content-explorer/ContentExplorer.tsx (1)
src/elements/content-explorer/utils.ts (1)
isMultiValuesField(74-82)
src/elements/content-explorer/MetadataSidePanel.tsx (2)
src/elements/content-explorer/utils.ts (1)
useTemplateInstance(85-151)src/elements/common/messages.js (1)
messages(9-1143)
src/elements/content-explorer/utils.ts (1)
src/elements/common/messages.js (1)
messages(9-1143)
🪛 Biome (2.1.2)
src/api/Metadata.js
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 814-814: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 824-824: Shouldn't redeclare 'id'. Consider to delete it or rename it.
'id' is defined here:
(lint/suspicious/noRedeclare)
⏰ 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 (12)
i18n/en-US.properties (2)
594-594: LGTM: “Multiple Values” copyClear and concise label for the multi-select, mixed-value state.
460-460: LGTM: ARIA labels for notification iconsAdding specific ARIA labels for the notification icons improves accessibility and matches usage in the UI.
Also applies to: 614-614, 846-846
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
11-11: ImportingFIELD_PERMISSIONSis correct and keeps tests in sync with the helper logic.src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
74-81: Good: unified empty-vs-non-empty handling viaisEmptyValue.The add/remove branching is straightforward and readable.
251-255: IncludingFIELD_PERMISSIONSby default is the right call.Ensures the UI can determine editability from each result without extra fetches.
src/api/Metadata.js (1)
19-20: ImportinggetTypedFolderIdis correct for folder PATCH support.This aligns the id typing for folders with files, preventing 4xx from mismatched typed ids.
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (1)
78-83: Nice: wrapping withNotification.Providermatches production tree for realistic UX assertions.src/elements/content-explorer/utils.ts (2)
73-82: LGTM: Multi-values sentinel detection is clear and consistentThe helper correctly distinguishes enum vs multiSelect sentinels and returns false otherwise.
117-124: Verify ReactNode support for value in @box/metadata-editor’s read-only fieldsPlease confirm that you can safely pass a React element as the
valueprop when rendering metadata in read-only mode and that no string-or-date parsing will be applied:• Open your installed package’s type declarations (e.g.
node_modules/@box/metadata-editor/index.d.ts) and verify that bothMetadataInstanceandMetadataTemplateFielddeclare theirvalueprop as acceptingReact.ReactNode(orReactNode/ReactElement).
• Inspect the implementation (innode_modules/@box/metadata-editor) to ensure that, in read-only mode, the component renders{value}directly or guards withReact.isValidElement(value)rather than always passing it through a parsing routine.
• Add a minimal smoke test in your codebase:import { MetadataInstance, MetadataTemplateField } from '@box/metadata-editor'; import { render, screen } from '@testing-library/react'; test('renders React element value without parsing', () => { render( <MetadataInstance /*…props*/> <MetadataTemplateField name="testField" value={<span data-testid="child">X</span>} readOnly /> </MetadataInstance> ); expect(screen.getByTestId('child')).toBeInTheDocument(); });If you find that
valueis typed as something likestring | numberor the implementation always invokes a parser, consider one of the following instead of overloadingvalue:
- Introduce a dedicated display‐only field in your template schema.
- Use the built-in renderer API (e.g. a custom
MetadataTemplateRenderer) to handle React nodes explicitly.src/elements/content-explorer/MetadataSidePanel.tsx (2)
78-88: LGTM: Success notification + state refreshGood use of success notification, edit-state reset, and explicit refresh to keep collection in sync.
90-98: LGTM: Error notification pathwayClear, accessible notifications for failure cases.
src/elements/content-explorer/ContentExplorer.tsx (1)
1853-2040: LGTM: Notification scaffolding and side-panel integration are cleanly wiredGood placement of Notification.Provider/Viewport, and the V2 metadata flow is gated behind features and default view checks. Prop drilling for getOperations and updateMetadataV2 is minimal and clear.
tjiang-box
force-pushed
the
multi-selection
branch
from
acdd3d8 to
ca750b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/elements/content-explorer/utils.ts (3)
42-47: Guard Number.isNaN with a typeof check (repeat).Calling Number.isNaN on a union-typed value yields false for non-numbers and raises TS errors. Gate it by typeof to ensure correctness and clean typing.
Apply:
export function isEmptyValue(value: ItemMetadataFieldValue) { if (isNil(value)) return true; - return value === '' || (Array.isArray(value) && value.length === 0) || Number.isNaN(value); + if (value === '') return true; + if (Array.isArray(value)) return value.length === 0; + return typeof value === 'number' && Number.isNaN(value); }
91-94: Defensive access: selection can be empty and metadata paths can be missing (repeat).Directly indexing selectedItems[0] and deep access into metadata will throw on empty selection or absent template/field.
- const firstSelectedItem = selectedItems[0]; - let fieldValue = firstSelectedItem.metadata[scope][templateKey][key]; + const firstSelectedItem = selectedItems[0]; + let fieldValue = + firstSelectedItem?.metadata?.[scope]?.[templateKey]?.[key] as ItemMetadataFieldValue | undefined;Additionally, consider early-returning a minimal template instance when selectedItems.length === 0.
109-113: Avoid mutating options on the original template (repeat).options?.push mutates the source template array; clone before adding MULTI_VALUE_DEFAULT_OPTION.
- const multiValueOption = options?.find(option => option.key === MULTI_VALUE_DEFAULT_VALUE); - if (!multiValueOption) { - options?.push(MULTI_VALUE_DEFAULT_OPTION); - } + const hasMultiValueOption = options?.some( + option => option.key === MULTI_VALUE_DEFAULT_VALUE, + ); + if (!hasMultiValueOption) { + options = options ? [...options, MULTI_VALUE_DEFAULT_OPTION] : [MULTI_VALUE_DEFAULT_OPTION]; + } else { + options = options ? [...options] : options; + }src/elements/content-explorer/ContentExplorer.tsx (2)
82-83: Import ErrorCallback for updateMetadataV2 typing (repeat).ErrorCallback is used in the method signature but not imported, causing a TS error.
-import type { JSONPatchOperations } from '../../common/types/api'; +import type { JSONPatchOperations, ErrorCallback } from '../../common/types/api';
498-550: Harden getOperations: type the accumulator, guard metadata access, and handle undefined oldField (repeat).
- operations is untyped.
- item.metadata[scope][templateKey] may be undefined.
- oldField can be undefined.
- Minor: fix “orignal” typo.
getOperations = ( item: BoxItem, templateOldFields: MetadataTemplateField[], templateNewFields: MetadataTemplateField[], ): JSONPatchOperations => { const { scope, templateKey } = this.state.metadataTemplate; - const itemFields = item.metadata[scope][templateKey]; - const operations = []; + const itemFields = getProp(item, `metadata.${String(scope)}.${String(templateKey)}`, {}) as Record< + string, + unknown + >; + const operations: JSONPatchOperations = []; templateNewFields.forEach(newField => { let newFieldValue = newField.value; const { key, type } = newField; // when retrieve value from float type field, it gives a string instead if (type === 'float' && newFieldValue !== '') { newFieldValue = Number(newFieldValue); } const oldField = templateOldFields.find(f => f.key === key); - const oldFieldValue = oldField.value; + const oldFieldValue = oldField ? oldField.value : undefined; let fieldOperations = []; /* - Generate operations array based on all the fields' orignal value and the incoming updated value. + Generate operations array based on all the fields' original value and the incoming updated value. @@ - if ( - isMultiValuesField(type as MetadataFieldType, oldFieldValue) && - !isMultiValuesField(type as MetadataFieldType, newFieldValue) - ) { + if ( + isMultiValuesField(type as MetadataFieldType, oldFieldValue as any) && + !isMultiValuesField(type as MetadataFieldType, newFieldValue as any) + ) { fieldOperations = this.metadataQueryAPIHelper.createJSONPatchOperations( key, itemFields[key], newFieldValue, ); } else { fieldOperations = this.metadataQueryAPIHelper.createJSONPatchOperations( key, oldFieldValue, newFieldValue, ); } operations.push(...fieldOperations); }); return operations; };
🧹 Nitpick comments (6)
src/elements/content-explorer/utils.ts (3)
58-63: Make multiSelect equality symmetric and duplicate-safe.length + every/includes is not robust with duplicates and is one-directional. Compare as sets.
- if (fieldType === 'multiSelect' && Array.isArray(value1) && Array.isArray(value2)) { - return value1.length === value2.length && value1.every(val => value2.includes(val)); - } + if (fieldType === 'multiSelect' && Array.isArray(value1) && Array.isArray(value2)) { + const s1 = new Set(value1); + const s2 = new Set(value2); + if (s1.size !== s2.size) return false; + for (const v of s1) { + if (!s2.has(v)) return false; + } + return true; + }
66-71: Use isEmptyValue instead of truthiness in setFieldValue.Truthiness can be surprising (e.g., 0). Align with isEmptyValue for consistency.
function setFieldValue(fieldType: MetadataFieldType, fieldValue: ItemMetadataFieldValue) { - if (fieldValue) return fieldValue; + if (!isEmptyValue(fieldValue)) return fieldValue; if (fieldType === 'multiSelect') return []; if (fieldType === 'enum') return ''; return fieldValue; }
94-128: Always normalize single-selection values via setFieldValue.For single selection, undefined enum/multiSelect values remain undefined. Normalize to ''/[] so the form controls render predictably.
- if (selectedItems.length > 1) { + if (selectedItems.length > 1) { // ...existing multi-selection logic... - } + } else { + fieldValue = setFieldValue(fieldType as MetadataFieldType, fieldValue); + }src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
240-269: Assert the “Multiple values” label appears after opening the panel.Strengthen the story by verifying the multi-selection UI state.
const metadataButton = canvas.getByRole('button', { name: 'Metadata' }); await userEvent.click(metadataButton); + + // Validate that the side panel reflects multiple values state + await waitFor(() => { + expect(screen.getByText(/Multiple values/i)).toBeInTheDocument(); + });src/api/Metadata.js (2)
108-121: Fix JSDoc to reflect folders and align comment text.The new helper targets folders; the JSDoc still says “base url for files”.
- /** - * API URL for metadata + /** + * API URL for metadata (folders) @@ - * @param {string} field - metadata field - * @return {string} base url for files + * @param {string} field - metadata field + * @return {string} base url for folders
813-866: Route by constants and guard unsupported types; keep cache update path.Prefer TYPE_FILE/TYPE_FOLDER to string literals and fail fast on unsupported types. This prevents accidental routing for weblinks.
- const metadata = await this.xhr.put({ - url: - item.type === 'file' - ? this.getMetadataUrl(id, template.scope, template.templateKey) - : this.getMetadataUrlForFolder(id, template.scope, template.templateKey), + const isFile = item.type === TYPE_FILE; + const isFolder = item.type === 'folder'; // import TYPE_FOLDER if available in constants + if (!isFile && !isFolder) { + throw new Error(`Unsupported item.type for metadata update: ${String(item.type)}`); + } + const metadata = await this.xhr.put({ + url: isFile + ? this.getMetadataUrl(id, template.scope, template.templateKey) + : this.getMetadataUrlForFolder(id, template.scope, template.templateKey), headers: { [HEADER_CONTENT_TYPE]: 'application/json-patch+json', }, - id: item.type === 'file' ? getTypedFileId(id) : getTypedFolderId(id), + id: isFile ? getTypedFileId(id) : getTypedFolderId(id), data: operations, });If TYPE_FOLDER exists in ../constants, import and use it for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
i18n/en-US.properties(4 hunks)src/api/Metadata.js(5 hunks)src/api/__tests__/Metadata.test.js(3 hunks)src/elements/common/messages.js(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(5 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(5 hunks)src/elements/content-explorer/MetadataSidePanel.tsx(5 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(2 hunks)src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx(3 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)src/elements/content-explorer/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/api/tests/Metadata.test.js
- src/elements/content-explorer/tests/MetadataSidePanel.test.tsx
- src/elements/content-explorer/tests/MetadataQueryAPIHelper.test.ts
- src/elements/common/messages.js
- src/elements/content-explorer/MetadataQueryAPIHelper.ts
- src/elements/content-explorer/MetadataSidePanel.tsx
- i18n/en-US.properties
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T16:19:21.964Z
Learnt from: greg-in-a-box
PR: box/box-ui-elements#4235
File: src/elements/content-explorer/MetadataQueryBuilder.ts:103-110
Timestamp: 2025-08-25T16:19:21.964Z
Learning: In src/elements/content-explorer/MetadataQueryBuilder.ts, the getSelectFilter function has a special case that maps 'mimetype-filter' to 'item.extension' because mimetype-filter is a multi-select field but the actual database field is 'item.extension'. This mapping is necessary and should not be removed.
Applied to files:
src/elements/content-explorer/utils.ts
🪛 Biome (2.1.2)
src/api/Metadata.js
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 814-814: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 824-824: Shouldn't redeclare 'id'. Consider to delete it or rename it.
'id' is defined here:
(lint/suspicious/noRedeclare)
⏰ 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 (3)
src/elements/content-explorer/ContentExplorer.tsx (2)
1865-1959: Nice integration of Notification.Provider/Viewport around the app chrome.Encapsulation looks good and keeps TooltipProvider scoping intact.
1949-1957: ✅ MetadataSidePanel prop types are correctly alignedThe
MetadataSidePanelPropsinterface insrc/elements/content-explorer/MetadataSidePanel.tsxdeclares:
getOperations(item: BoxItem, templateOldFields: MetadataTemplateField[], templateNewFields: MetadataTemplateField[]): JSONPatchOperations;updateMetadataV2(file: BoxItem, operations: JSONPatchOperations, successCallback?: () => void, errorCallback?: ErrorCallback): Promise<void>;selectedItemIds: Selection;These signatures exactly match the props passed in
ContentExplorer.tsx(lines 1949–1957). No discrepancies were found—no further updates are required here.src/api/Metadata.js (1)
813-866: Overall: Folder metadata support + typed ID wiring looks solid.URL construction and cache merge behavior mirror the file path; good parity.
jfox-box
left a comment
jfox-box
left a comment
There was a problem hiding this comment.
Nice work, some comments
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
63-71: Array equality via xor ignores duplicates; compare as multisets to be safe.Using lodash xor treats arrays as sets and misses differences in element multiplicity. While multiselects typically avoid duplicates, hardening here avoids subtle bugs and reduces O(n^2) worst-cases.
Apply this diff:
- if ( - (isEmptyValue(oldValue) && isEmptyValue(newValue)) || - (Array.isArray(oldValue) && Array.isArray(newValue) && xor(oldValue, newValue).length === 0) || - oldValue === newValue - ) { + if (isEmptyValue(oldValue) && isEmptyValue(newValue)) { + return []; + } + if (Array.isArray(oldValue) && Array.isArray(newValue)) { + if (oldValue.length === newValue.length) { + const counts = new Map<any, number>(); + for (const v of oldValue) counts.set(v, (counts.get(v) || 0) + 1); + let equal = true; + for (const v of newValue) { + const c = counts.get(v); + if (!c) { equal = false; break; } + counts.set(v, c - 1); + } + if (equal && Array.from(counts.values()).every(x => x === 0)) { + return []; + } + } + } + if (oldValue === newValue) { return []; - } + }
265-274: Propagate errors and short-circuit empty ops to avoid silent failures and 400s.Without callbacks, updateMetadata resolves even on failure; multi-item flow won’t reach catch. Also, PATCHing an empty op array is invalid for many APIs.
Apply this diff:
- ): Promise<void> => { - return this.api - .getMetadataAPI(true) - .updateMetadata(item, this.metadataTemplate, operations, successCallback, errorCallback); - }; + ): Promise<void> => { + // No-ops: resolve and invoke success immediately + if (!operations || operations.length === 0) { + if (successCallback) successCallback(); + return Promise.resolve(); + } + // Wrap in a promise that rejects on error to allow callers to catch + return new Promise((resolve, reject) => { + this.api + .getMetadataAPI(true) + .updateMetadata( + item, + this.metadataTemplate, + operations, + () => { + if (successCallback) successCallback(); + resolve(); + }, + (err) => { + if (errorCallback) errorCallback(err); + reject(err); + }, + ); + }); + };src/elements/content-explorer/MetadataSidePanel.tsx (1)
25-41: Import missing ErrorCallback type and unify JSONPatchOperations source.ErrorCallback isn’t imported; this will fail type-checking. Also, JSONPatchOperations is sourced from @box/metadata-editor here but from ../../common/types/api elsewhere; unify on the shared API types for consistency.
Apply this diff:
-import { - AutofillContextProvider, - FormValues, - JSONPatchOperations, - MetadataInstance, - MetadataInstanceForm, - type MetadataTemplateField, -} from '@box/metadata-editor'; +import { + AutofillContextProvider, + FormValues, + MetadataInstance, + MetadataInstanceForm, + type MetadataTemplateField, +} from '@box/metadata-editor'; +import type { JSONPatchOperations, ErrorCallback } from '../../common/types/api';src/elements/content-explorer/utils.ts (3)
136-141: Avoid mutating the template’s options array (immutability)Pushing into options mutates the original template and can leak across renders. Clone before adding MULTI_VALUE_DEFAULT_OPTION; also clone when already present to avoid external mutation surprises.
- const multiValueOption = options?.find(option => option.key === MULTI_VALUE_DEFAULT_VALUE); - if (!multiValueOption) { - options?.push(MULTI_VALUE_DEFAULT_OPTION); - } + const hasMultiValueOption = options?.some(option => option.key === MULTI_VALUE_DEFAULT_VALUE); + if (!hasMultiValueOption) { + options = options + ? [...options, MULTI_VALUE_DEFAULT_OPTION] + : [MULTI_VALUE_DEFAULT_OPTION]; + } else if (options) { + // Still clone to avoid leaking mutations + options = [...options]; + }
59-63: Fix NaN check to avoid TS type error and false negativesNumber.isNaN on a union-typed value causes a TS error and incorrectly returns false for non-number types. Gate it by typeof before calling Number.isNaN.
- // float - if (Number.isNaN(value)) { + // float + if (typeof value === 'number' && Number.isNaN(value)) { return true; }
120-123: Guard against empty selection and missing metadata path (prevents runtime errors)selectedItems[0] and deep metadata access can be undefined. Optional-chain the lookups and provide a safe default for empty selection; also guard in the .every() comparator.
- const firstSelectedItem = selectedItems[0]; - let fieldValue = firstSelectedItem.metadata[scope][templateKey][key]; + const firstSelectedItem = selectedItems[0]; + let fieldValue = + (firstSelectedItem?.metadata?.[scope]?.[templateKey]?.[key] as ItemMetadataFieldValue | undefined); + + // Empty selection safety: default the field value for rendering/editing + if (!firstSelectedItem) { + fieldValue = setFieldValue(fieldType as MetadataFieldType, undefined); + } - const allItemsHaveSameInitialValue = selectedItems.every(selectedItem => - areFieldValuesEqual( - fieldType as MetadataFieldType, - selectedItem.metadata[scope][templateKey][key], - fieldValue, - ), - ); + const allItemsHaveSameInitialValue = selectedItems.every(selectedItem => { + const itemValue = selectedItem?.metadata?.[scope]?.[templateKey]?.[key] as + | ItemMetadataFieldValue + | undefined; + return areFieldValuesEqual(fieldType as MetadataFieldType, itemValue, fieldValue); + });Also applies to: 124-131
🧹 Nitpick comments (12)
i18n/en-US.properties (2)
585-588: Trim leading/trailing spaces in success notification to avoid stray UI whitespace.There are extra spaces before and after the ICU block. This can render unintended padding in some contexts and makes diffs noisy.
Apply this diff:
-be.metadataUpdateSuccessNotification = {numSelected, plural, =1 {1 document updated} other {# documents updated} } +be.metadataUpdateSuccessNotification = {numSelected, plural, =1 {1 document updated} other {# documents updated}}
616-616: Remove surrounding spaces from pluralized selection string.The current value has a leading and trailing space around the ICU block; this can introduce layout inconsistencies.
Apply this diff:
-be.numFilesSelected = {numSelected, plural, =0 {0 files selected} one {1 file selected} other {# files selected} } +be.numFilesSelected = {numSelected, plural, =0 {0 files selected} one {1 file selected} other {# files selected}}src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (3)
149-166: Strengthen assertion by verifying operations type and callbacks presence.You already assert the shape for single-item submit. For completeness, also assert that the second and third args are functions (success and error callbacks) to prevent regressions in the onUpdate contract.
Example enhancement:
- expect(mockUpdateMetadata).toHaveBeenCalledWith( + expect(mockUpdateMetadata).toHaveBeenCalledWith( mockCollection.items[0], expect.any(Array), expect.any(Function), expect.any(Function), ); + const [, ops, onSuccess, onError] = mockUpdateMetadata.mock.calls[0]; + expect(Array.isArray(ops)).toBe(true); + expect(typeof onSuccess).toBe('function'); + expect(typeof onError).toBe('function');
167-187: Also assert argument values for getOperations and onUpdate to guard wiring.Currently you only check call counts. Verifying that getOperations receives old/new fields and that onUpdate gets the returned ops reduces the risk of wiring mistakes.
Suggested additions:
- await waitFor(() => { - expect(mockGetOperations).toHaveBeenCalledTimes(2); - expect(mockUpdateMetadata).toHaveBeenCalledTimes(2); - }); + await waitFor(() => { + expect(mockGetOperations).toHaveBeenCalledTimes(2); + expect(mockUpdateMetadata).toHaveBeenCalledTimes(2); + }); + // Ensure getOperations called with (item, oldFields, newFields) + const [firstCallItem, firstCallOld, firstCallNew] = mockGetOperations.mock.calls[0]; + expect(firstCallItem.id).toBe('1'); + expect(Array.isArray(firstCallOld)).toBe(true); + expect(Array.isArray(firstCallNew)).toBe(true); + // Ensure onUpdate receives the ops returned by getOperations + const opsFromGet = mockGetOperations.mock.results[0].value; + expect(mockUpdateMetadata).toHaveBeenNthCalledWith( + 1, + mockCollection.items[0], + opsFromGet, + expect.anything(), + expect.anything(), + );
233-252: Add a test for short-circuit and error propagation in multi-item flow.Two edge cases worth locking down:
- No-op updates (empty ops array) should not call onUpdate (see helper-level early return suggestion).
- If one item fails, the loop should stop and show the error banner.
Both will catch regressions in the reduce-based sequencing.
If helpful, I can add a test that makes the first onUpdate reject and asserts second isn’t called and that the error banner appears.
src/api/Metadata.js (2)
108-121: Fix JSDoc for folder metadata URL helper.Comments reference “files” and a “field” param that isn’t in the signature. Update to reflect folders and the actual params (scope, template).
Apply this diff:
- /** - * API URL for metadata - * - * @param {string} id - a Box folder id - * @param {string} field - metadata field - * @return {string} base url for files - */ + /** + * API URL for folder metadata. + * + * @param {string} id - Box folder id + * @param {string} [scope] - metadata scope + * @param {string} [template] - metadata template key + * @return {string} base url for folders + */
803-868: Routing metadata updates by item.type looks correct; consider minor cleanup.
- Nice: selecting URL and typed id based on file vs folder.
- Minor: destructure type alongside id, permissions for consistency.
Apply this diff:
- const { id, permissions } = item; + const { id, permissions, type } = item; ... - const { type } = item; const metadata = await this.xhr.put({ - url: - type === 'file' + url: + type === 'file' ? this.getMetadataUrl(id, template.scope, template.templateKey) : this.getMetadataUrlForFolder(id, template.scope, template.templateKey), ... - id: type === 'file' ? getTypedFileId(id) : getTypedFolderId(id), + id: type === 'file' ? getTypedFileId(id) : getTypedFolderId(id),Also consider an item-aware helper (e.g., getItemMetadataUrl(item, scope, template)) to centralize this branching.
src/elements/content-explorer/MetadataSidePanel.tsx (1)
155-156: Avoid passing null handlers; omit the prop to reduce null-check risk.Passing null can trip consumers that call the handler without guarding. Prefer omitting the prop or using undefined.
Apply this diff:
- onChange={null} + // onChange intentionally omittedsrc/elements/content-explorer/utils.ts (4)
76-78: Make multiSelect equality order-agnostic without O(n²) includesCurrent every/includes is quadratic and can produce false positives when duplicates are present. A set-based check is simpler and faster assuming multiSelects cannot contain duplicates. If duplicates are possible, use a multiset/frequency map instead.
- return value1.length === value2.length && value1.every(val => value2.includes(val)); + const s1 = new Set(value1); + const s2 = new Set(value2); + return s1.size === s2.size && [...s1].every(v => s2.has(v));If duplicates can appear, swap the above for a frequency-map comparison. Happy to provide that version.
165-165: Normalize return value through setFieldValue for single-selection parityFor single selection, undefined values currently pass through as undefined. Routing all returns through setFieldValue ensures defaults are applied consistently in view/edit modes.
- value: fieldValue, + value: setFieldValue(fieldType as MetadataFieldType, fieldValue),Please sanity-check UI behavior for empty single-selection fields (float/date/enum/string) after this change.
146-153: Verify the consumer accepts React elements as field valuesUsing a Fragment to bypass date parsing is clever, but ensure the downstream field “value” type supports React.ReactNode (not just string/array). If not, consider a dedicated flag or displayValue API to avoid type drift.
I can help refactor to a displayValue/render prop if MetadataFormFieldValue doesn’t allow React nodes.
42-66: Add tests for helper semantics and multi-selection edge casesThe new helpers and multi-selection path would benefit from unit tests:
- isEmptyValue: null/undefined/''/[]/0/NaN per field type.
- areFieldValuesEqual: multiSelect order differences and (if possible) duplicate handling; empty vs undefined equivalence.
- useTemplateInstance:
- empty selection safety,
- differing values in edit vs view modes (enum/multiSelect vs other types),
- options immutability when injecting MULTI_VALUE_DEFAULT_OPTION,
- single vs multi selection defaulting via setFieldValue.
I can draft these tests targeting utils.ts and a few integration cases in MetadataSidePanel.
Also applies to: 67-81, 102-111, 114-179
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
i18n/en-US.properties(2 hunks)src/api/Metadata.js(5 hunks)src/elements/common/messages.js(2 hunks)src/elements/content-explorer/ContentExplorer.tsx(4 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(6 hunks)src/elements/content-explorer/MetadataSidePanel.tsx(5 hunks)src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx(3 hunks)src/elements/content-explorer/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/content-explorer/ContentExplorer.tsx
- src/elements/common/messages.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T16:19:21.964Z
Learnt from: greg-in-a-box
PR: box/box-ui-elements#4235
File: src/elements/content-explorer/MetadataQueryBuilder.ts:103-110
Timestamp: 2025-08-25T16:19:21.964Z
Learning: In src/elements/content-explorer/MetadataQueryBuilder.ts, the getSelectFilter function has a special case that maps 'mimetype-filter' to 'item.extension' because mimetype-filter is a multi-select field but the actual database field is 'item.extension'. This mapping is necessary and should not be removed.
Applied to files:
src/elements/content-explorer/MetadataQueryAPIHelper.tssrc/elements/content-explorer/utils.ts
🧬 Code graph analysis (4)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
src/elements/content-explorer/utils.ts (2)
isEmptyValue(44-65)isMultiValuesField(103-111)src/features/metadata-based-view/MetadataQueryAPIHelper.js (2)
operation(66-66)clonedFields(235-235)
src/elements/content-explorer/utils.ts (1)
src/elements/common/messages.js (1)
messages(9-1133)
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (2)
src/elements/content-explorer/MetadataSidePanel.tsx (1)
MetadataSidePanelProps(25-42)src/elements/content-explorer/ContentExplorer.tsx (1)
render(1736-2015)
src/elements/content-explorer/MetadataSidePanel.tsx (2)
src/elements/content-explorer/utils.ts (1)
useTemplateInstance(114-180)src/elements/common/messages.js (1)
messages(9-1133)
🪛 Biome (2.1.2)
src/api/Metadata.js
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 814-814: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 824-824: Shouldn't redeclare 'id'. Consider to delete it or rename it.
'id' is defined here:
(lint/suspicious/noRedeclare)
⏰ 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)
i18n/en-US.properties (2)
595-596: LGTM: “Multiple Values” copy is clear and matches UI usage.
843-844: LGTM: “Success” label addition aligns with notification semantics.src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (1)
77-83: Good coverage: wrapping renders in Notification context ensures banners are testable.src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
74-81: LGTM: ADD/REMOVE decision based on emptiness is clearer and handles more types safely.
296-300: LGTM: including FIELD_PERMISSIONS in query fields enables permission checks in the editor.src/elements/content-explorer/MetadataSidePanel.tsx (2)
78-91: LGTM: success path notification + refresh + mode reset is user-friendly and matches tests.
102-121: Incorrect helper reference and ensure failure propagationThe suggested “helper-centric” path refers to a non-existent
updateMetadataWithOperationsfunction inMetadataQueryAPIHelper.ts. To properly catch per-item failures in the multi-item branch, you still need to guarantee that theonUpdatefunction (which is passed intoMetadataSidePanel) rejects its Promise on error or explicitly throws. Two straightforward options:• Modify the upstream
onUpdateimplementation (wherever it’s defined) so that it returns a Promise which rejects on any API error. This ensures thecatchblock here is reached without any changes to this callsite.
• Or, at this callsite, pass an error callback that throws—forcing a rejection if an individual item update fails:- await onUpdate(item, ops); + await onUpdate( + item, + ops, + /* onSuccess */ undefined, + () => { + throw new Error(`Metadata update failed for item ${item.id}`); + } + );Either approach works; pick the one that best fits your existing error-handling pattern.
Likely an incorrect or invalid review comment.
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx
Outdated
Show resolved
Hide resolved
tjiang-box
force-pushed
the
multi-selection
branch
from
90f0458 to
5ee953c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (1)
89-93: Redundant a11y attribute tests; remove or repurpose.These validate 3rd-party components’ aria-label behavior, not our logic. Prefer using the labels only as selectors in functional tests.
Also applies to: 272-282
src/elements/content-explorer/MetadataSidePanel.tsx (1)
29-36: ErrorCallback is not in scope here; also unify JSONPatchOperations type source.This will fail type-checking at compile time. Import a shared ErrorCallback and JSONPatchOperations from the common API types (and stop sourcing JSONPatchOperations from metadata-editor in this file) to avoid duplicate/structurally-incompatible types across files.
-import { - AutofillContextProvider, - FormValues, - JSONPatchOperations, - MetadataInstance, - MetadataInstanceForm, - type MetadataTemplateField, -} from '@box/metadata-editor'; +import { + AutofillContextProvider, + FormValues, + MetadataInstance, + MetadataInstanceForm, + type MetadataTemplateField, +} from '@box/metadata-editor'; +import type { JSONPatchOperations, ErrorCallback } from '../../common/types/api';Also applies to: 8-14
🧹 Nitpick comments (11)
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (5)
148-166: Stabilize async assertion for single-item save.onUpdate is async; assert within waitFor to avoid flakiness.
- expect(mockUpdateMetadata).toHaveBeenCalledWith( - [mockCollection.items[0]], - expect.any(Array), - expect.any(Array), - expect.any(Array), - expect.any(Function), - expect.any(Function), - ); + await waitFor(() => { + expect(mockUpdateMetadata).toHaveBeenCalledWith( + [mockCollection.items[0]], + expect.any(Array), + expect.any(Array), + expect.any(Array), + expect.any(Function), + expect.any(Function), + ); + });
168-185: Verify onUpdate args for multi-select.Also assert the items array contains both selected entries.
- await waitFor(() => { - expect(mockUpdateMetadata).toHaveBeenCalledTimes(1); - }); + await waitFor(() => { + expect(mockUpdateMetadata).toHaveBeenCalledWith( + [mockCollection.items[0], mockCollection.items[1]], + expect.any(Array), + expect.any(Array), + expect.any(Array), + expect.any(Function), + expect.any(Function), + ); + });
187-210: Nice coverage of success path.Consider also asserting absence of Save/Cancel after success for clarity.
212-229: Error path covered; add a no-refresh assertion.Prevent regressions by asserting refreshCollection was not called.
await waitFor(() => { expect(screen.getByText('Unable to save changes. Please try again')).toBeInTheDocument(); + expect(defaultProps.refreshCollection).not.toHaveBeenCalled(); });
66-74: Reset mocks between tests.defaultProps contain jest fns; add a global reset to prevent cross-test leakage.
describe('elements/content-explorer/MetadataSidePanel', () => { + afterEach(() => { + jest.clearAllMocks(); + });src/elements/content-explorer/ContentExplorer.tsx (1)
496-527: Guard and contain thrown errors from helpers.If helpers throw (in addition to invoking errorCallback), updateMetadataV2 will reject and may bubble to UI. Catch and route to errorCallback.
- updateMetadataV2 = async ( + updateMetadataV2 = async ( items: BoxItem[], operations: JSONPatchOperations, templateOldFields: MetadataTemplateField[], templateNewFields: MetadataTemplateField[], successCallback: () => void, errorCallback: ErrorCallback, ) => { - if (items.length === 1) { - await this.metadataQueryAPIHelper.updateMetadataWithOperations( - items[0], - operations, - successCallback, - errorCallback, - ); - } else { - await this.metadataQueryAPIHelper.bulkUpdateMetadata( - items, - templateOldFields, - templateNewFields, - successCallback, - errorCallback, - ); - } + if (!items?.length) return; + try { + if (items.length === 1) { + await this.metadataQueryAPIHelper.updateMetadataWithOperations( + items[0], + operations, + successCallback, + errorCallback, + ); + } else { + await this.metadataQueryAPIHelper.bulkUpdateMetadata( + items, + templateOldFields, + templateNewFields, + successCallback, + errorCallback, + ); + } + } catch (e) { + errorCallback?.(e as Error); + } };src/api/__tests__/Metadata.test.js (1)
1901-2000: Solid bulkUpdateMetadata coverage. Add folder-route test.Add a case with a folder item to assert use of getMetadataUrlForFolder/getTypedFolderId and mixed file/folder batches.
src/api/Metadata.js (3)
108-122: Doc nits for folder URL helper.Update JSDoc to say “API URL for folder metadata” and “base url for folders”.
- * API URL for metadata + * API URL for folder metadata @@ - * @return {string} base url for files + * @return {string} base url for folders
880-923: Consider returning a result summary from bulkUpdateMetadata.Returning { successCount, failureCount, errors } would enable precise UI messaging and retries. Current design only uses callbacks.
844-853: Type guard for unsupported types.If item.type is neither 'file' nor 'folder', defaulting to folder is risky. Guard and error early.
- url: - type === 'file' - ? this.getMetadataUrl(id, template.scope, template.templateKey) - : this.getMetadataUrlForFolder(id, template.scope, template.templateKey), + url: + type === 'file' + ? this.getMetadataUrl(id, template.scope, template.templateKey) + : type === 'folder' + ? this.getMetadataUrlForFolder(id, template.scope, template.templateKey) + : (() => { throw getBadItemError(); })(), @@ - id: type === 'file' ? getTypedFileId(id) : getTypedFolderId(id), + id: type === 'file' ? getTypedFileId(id) : type === 'folder' ? getTypedFolderId(id) : id,src/elements/content-explorer/MetadataSidePanel.tsx (1)
143-144: Don’t pass null to optional callbacks; prefer undefined or omit the prop.Passing null can violate the component’s prop types at runtime/compile time. Use undefined instead.
- onChange={null} + onChange={undefined}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/api/Metadata.js(5 hunks)src/api/__tests__/Metadata.test.js(11 hunks)src/elements/content-explorer/ContentExplorer.tsx(4 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(6 hunks)src/elements/content-explorer/MetadataSidePanel.tsx(5 hunks)src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-27T15:25:53.190Z
Learnt from: jpan-box
PR: box/box-ui-elements#4248
File: src/elements/content-explorer/MetadataViewContainer.tsx:30-44
Timestamp: 2025-08-27T15:25:53.190Z
Learning: In the Box API, there is an inconsistency where some endpoints require trimmed metadata field names (e.g., "industry") while others require the full field path (e.g., "metadata.enterprise_1515946.templateName.industry"). The trimMetadataFieldPrefix helper function in MetadataViewContainer.tsx correctly trims the field names for API endpoints that expect the shorter format.
Applied to files:
src/elements/content-explorer/MetadataQueryAPIHelper.ts
📚 Learning: 2025-08-25T16:19:22.007Z
Learnt from: greg-in-a-box
PR: box/box-ui-elements#4235
File: src/elements/content-explorer/MetadataQueryBuilder.ts:103-110
Timestamp: 2025-08-25T16:19:22.007Z
Learning: In src/elements/content-explorer/MetadataQueryBuilder.ts, the getSelectFilter function has a special case that maps 'mimetype-filter' to 'item.extension' because mimetype-filter is a multi-select field but the actual database field is 'item.extension'. This mapping is necessary and should not be removed.
Applied to files:
src/elements/content-explorer/MetadataQueryAPIHelper.ts
🧬 Code graph analysis (5)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
src/elements/content-explorer/utils.ts (2)
isEmptyValue(44-65)isMultiValuesField(103-111)
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (1)
src/elements/content-explorer/MetadataSidePanel.tsx (1)
MetadataSidePanelProps(25-39)
src/elements/content-explorer/ContentExplorer.tsx (1)
src/elements/common/upload-dialog/UploadDialog.js (1)
UploadDialog(35-76)
src/elements/content-explorer/MetadataSidePanel.tsx (2)
src/elements/content-explorer/utils.ts (1)
useTemplateInstance(114-180)src/elements/common/messages.js (1)
messages(9-1133)
src/api/__tests__/Metadata.test.js (1)
src/constants.js (2)
ERROR_CODE_UPDATE_METADATA(330-330)ERROR_CODE_UPDATE_METADATA(330-330)
🪛 Biome (2.1.2)
src/api/Metadata.js
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 815-815: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 816-816: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 817-817: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 818-818: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 819-819: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 819-819: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 820-820: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 820-820: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 821-821: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 830-830: Shouldn't redeclare 'id'. Consider to delete it or rename it.
'id' is defined here:
(lint/suspicious/noRedeclare)
[error] 830-830: Shouldn't redeclare 'type'. Consider to delete it or rename it.
'type' is defined here:
(lint/suspicious/noRedeclare)
[error] 891-891: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 892-892: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 893-893: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 894-894: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 895-895: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 896-896: return 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 (9)
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx (2)
76-82: Good move: wrap under Notification provider.This ensures toast assertions are reliable and mirrors runtime composition.
231-235: Selection/multi-value behaviors look good.Also applies to: 237-270
src/elements/content-explorer/ContentExplorer.tsx (2)
1821-2008: Provider/viewport wiring LGTM.The Notification/Tooltip layering mirrors the tests and enables toasts globally.
496-527: metadataQueryAPIHelper is always initialized before updateMetadataV2The side panel’s
onUpdate={this.updateMetadataV2}is only rendered when all of the following are true:
- The current view is the default metadata view
- The metadata-view-v2 feature flag is enabled
- The side panel is open
The feature flag and view check guarantee that
showMetadataQueryResults()has already executed, which synchronously setsthis.metadataQueryAPIHelper(either toMetadataQueryAPIHelperV2orMetadataQueryAPIHelper) before any user interaction can open the panel. As a result,updateMetadataV2will always have a valid helper instance and no additional guard is needed.src/api/__tests__/Metadata.test.js (1)
1657-1691: Tests updated for new updateMetadata signature and file types — good.Also applies to: 1704-1763, 1773-1831, 1840-1899
src/api/Metadata.js (1)
814-878: Update path routing by item.type looks correct; cache update guarded.src/elements/content-explorer/MetadataSidePanel.tsx (1)
74-86: Notification UX wiring looks solid.Success/error toasts use proper i18n labels and pluralization; refreshing the collection post-success is correct.
Also applies to: 88-96
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
63-71: Equality fast-path is good.Using isEmptyValue and xor to short-circuit no-op patches improves correctness and avoids unnecessary API calls.
313-317: Including FIELD_PERMISSIONS is necessary; good catch.Ensures callers can check update capability before attempting writes.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/Metadata.js (1)
860-866: Bug: splice(-1, …) will clobber the last editor when id not found.
IffindIndexreturns -1, you replace the last entry. Don’t do your cache like that, fool.Apply this fix:
- if (cachedMetadata && cachedMetadata.editors) { - cachedMetadata.editors.splice( - cachedMetadata.editors.findIndex(({ instance }) => instance.id === editor.instance.id), - 1, - editor, - ); - } + if (cachedMetadata && Array.isArray(cachedMetadata.editors)) { + const idx = cachedMetadata.editors.findIndex( + ({ instance }) => instance.id === editor.instance.id, + ); + if (idx > -1) { + cachedMetadata.editors.splice(idx, 1, editor); + } else { + cachedMetadata.editors.push(editor); + } + }
♻️ Duplicate comments (1)
src/api/Metadata.js (1)
830-830: Past feedback addressed: destructuringtype.
You pulledtypeoffitemas requested. All good.
🧹 Nitpick comments (6)
src/api/Metadata.js (2)
1-5: Biome complaints are false alarms for Flow code, fool.
This file is Flow (// @flow). Biome’s TS parse errors can be ignored or silenced via config/ignore for Flow-typed JS.If lint noise is breaking CI, I can propose a
.biome.jsonoverride or add a pre-lint flow-strip step. Want me to open a small infra PR?
108-121: Fix JSDoc to say “folder” not “file”.
Doc says “base url for files” but method targets folders. Keep your docs honest, sucka.Apply this tweak:
/** - * API URL for metadata + * API URL for folder metadata * - * @param {string} id - a Box folder id - * @param {string} field - metadata field - * @return {string} base url for files + * @param {string} id - a Box folder id + * @param {string} field - metadata field + * @return {string} base url for folders */Also, add a unit test for
getMetadataUrlForFolder(id, scope, template)mirroring the existing file URL tests. I pity the fool who ships untested URL helpers.src/api/__tests__/Metadata.test.js (4)
1945-1973: Bulk-failure test is fine; consider asserting context.
You check the aggregated message; you might also assert it includes the failing item id to help users.
1652-1898: Add folder-routing coverage forupdateMetadata.
Test the new folder path and typed id, fool. Suggested test:test('should route to folder URL with typed folder id', async () => { const success = jest.fn(); const error = jest.fn(); const folder = { id: '123', permissions: { can_upload: true }, type: 'folder' }; const ops = [{ op: 'replace', path: '/foo', value: 'bar' }]; const template = { scope: 'scope', templateKey: 'templateKey' }; const cache = new Cache(); metadata.getMetadataUrlForFolder = jest.fn().mockReturnValueOnce('folder_url'); metadata.xhr.put = jest.fn().mockResolvedValueOnce({ data: { $id: 'inst', $canEdit: true } }); metadata.isDestroyed = jest.fn().mockReturnValueOnce(false); metadata.getCache = jest.fn().mockReturnValueOnce(cache); metadata.getMetadataCacheKey = jest.fn().mockReturnValueOnce('metadata_123'); metadata.createEditor = jest.fn().mockReturnValueOnce({ instance: { id: 'inst' } }); metadata.successHandler = jest.fn(); await metadata.updateMetadata(folder, template, ops, success, error); expect(metadata.getMetadataUrlForFolder).toHaveBeenCalledWith(folder.id, 'scope', 'templateKey'); expect(metadata.xhr.put).toHaveBeenCalledWith({ url: 'folder_url', headers: { 'Content-Type': 'application/json-patch+json' }, id: 'folder_123', data: ops, }); expect(metadata.successHandler).toHaveBeenCalled(); });
1652-1898: Cover the cache splice edge case.
Prove we push when the instance isn’t yet present, not clobber the last entry, fool.test('updateMetadata pushes editor when instance id not found', async () => { const success = jest.fn(); const error = jest.fn(); const file = { id: 'id', permissions: { can_upload: true }, type: 'file' }; const ops = []; const template = { scope: 'scope', templateKey: 'templateKey' }; const cache = new Cache(); cache.set('metadata_id', { editors: [{ instance: { id: 'other' } }] }); metadata.getMetadataUrl = jest.fn().mockReturnValueOnce('url'); metadata.xhr.put = jest.fn().mockResolvedValueOnce({ data: { $id: 'new' } }); metadata.isDestroyed = jest.fn().mockReturnValueOnce(false); metadata.getCache = jest.fn().mockReturnValueOnce(cache); metadata.getMetadataCacheKey = jest.fn().mockReturnValueOnce('metadata_id'); metadata.createEditor = jest.fn().mockReturnValueOnce({ instance: { id: 'new' } }); metadata.successHandler = jest.fn(); await metadata.updateMetadata(file, template, ops, success, error); expect(cache.get('metadata_id').editors.map(e => e.instance.id)).toEqual(['other', 'new']); });
1900-2015: Consider a test for mismatched operations length (if you add the guard).
If you adopt the length check, add a failing test assertingerrorHandleris called with the mismatch message. I can drop in the exact test once you confirm.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP (Model Context Protocol) integration is disabled
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/api/Metadata.js(5 hunks)src/api/__tests__/Metadata.test.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/api/__tests__/Metadata.test.js (1)
src/constants.js (2)
ERROR_CODE_UPDATE_METADATA(330-330)ERROR_CODE_UPDATE_METADATA(330-330)
🪛 Biome (2.1.2)
src/api/Metadata.js
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 115-115: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 815-815: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 816-816: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 817-817: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 818-818: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 819-819: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 820-820: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 820-820: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 821-821: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 830-830: Shouldn't redeclare 'id'. Consider to delete it or rename it.
'id' is defined here:
(lint/suspicious/noRedeclare)
[error] 830-830: Shouldn't redeclare 'type'. Consider to delete it or rename it.
'type' is defined here:
(lint/suspicious/noRedeclare)
[error] 891-891: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 892-892: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 893-893: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 894-894: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 895-895: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 896-896: return 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 (6)
src/api/Metadata.js (3)
19-19: Good addition: folder-typed ID helper imported.
UsinggetTypedFolderIdis the right call for folder routes. Nice.
815-829: Callback suppression logic is clean.
Conditionally wiringsuccessCallback/errorCallbackonly when needed is the right move for bulk flows.
845-854: Guard Against InvalidtypeValues in Metadata API RoutingI pity the fool who lets an unexpected item type slip through! The code currently treats any non‐
'file'value as a folder, so if someone ever passes a'web_link'or any other type, it’ll quietly fall into the folder branch. No evidence of other types in the codebase so far, but you’ll want to be sure:• Confirm that everywhere
typeis set or passed in, it’s always just'file'or'folder'.
• If you ever accept new item types (e.g.'web_link','shortcut', etc.), add a fail‐fast guard instead of silently defaulting to folder.Optional refactor: replace your ternaries with an explicit check and throw on unsupported values:
- url: type === 'file' - ? this.getMetadataUrl(...) - : this.getMetadataUrlForFolder(...), + url: + type === 'file' + ? this.getMetadataUrl(...) + : type === 'folder' + ? this.getMetadataUrlForFolder(...) + : (() => { throw new Error(`Unsupported item.type "${type}"`); })(),…and similarly for
id:- id: type === 'file' ? getTypedFileId(id) : getTypedFolderId(id), + id: + type === 'file' + ? getTypedFileId(id) + : type === 'folder' + ? getTypedFolderId(id) + : getTypedFileId(id), // unreachable if you throw above; keeps Flow happyI pity the fool who skips this step!
src/api/__tests__/Metadata.test.js (3)
1703-1704: Good: explicittype: 'file'in update tests.
Keeps the routing logic unambiguous.Also applies to: 1771-1772, 1838-1839
1901-1943: Bulk-success test reads solid.
Asserts per-item calls withtruesuppression and single success notification.
1974-1993: Destroyed-state bulk tests look good.
No callbacks when destroyed — exactly right.Also applies to: 1995-2014
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx
Outdated
Show resolved
Hide resolved
tjiang-box
force-pushed
the
multi-selection
branch
from
0bec253 to
a86ca94
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
src/api/Metadata.js (1)
914-946: Bulk update: add precondition and aggregate failures, fool.Guard length mismatch and return all per-item errors with context via allSettled. Don’t make folks chase ghosts.
Apply this diff:
async bulkUpdateMetadata( items: BoxItem[], template: MetadataTemplate, operations: JSONPatchOperations[], successCallback: Function, errorCallback: ElementsErrorCallback, ): Promise<void> { this.errorCode = ERROR_CODE_UPDATE_METADATA; - this.successCallback = successCallback; - this.errorCallback = errorCallback; + this.successCallback = successCallback; + this.errorCallback = errorCallback; + + // Precondition: items and operations must align + if (items.length !== operations.length) { + const msg = `items (${items.length}) and operations (${operations.length}) length mismatch`; + return this.errorHandler(new Error(msg)); + } try { - const updatePromises = items.map(async (item, index) => { - try { - // Suppress per-item callbacks; aggregate outcome at the bulk level only - await this.updateMetadata(item, template, operations[index], successCallback, errorCallback, true); - } catch (e) { - // Re-throw to be caught by Promise.all and handled once below - throw new Error(`Failed to update metadata: ${e.message || e}`); - } - }); - - await Promise.all(updatePromises); + const results = await Promise.allSettled( + items.map((item, index) => + this.updateMetadata(item, template, operations[index], successCallback, errorCallback, true), + ), + ); + + const failures = results + .map((r, i) => (r.status === 'rejected' ? { index: i, id: items[i]?.id, error: r.reason } : null)) + .filter(Boolean); + + if (failures.length > 0) { + const agg = new AggregateError( + failures.map(f => new Error(`item ${f.id ?? f.index}: ${String(f.error)}`)), + `bulkUpdateMetadata failed for ${failures.length}/${items.length} item(s)`, + ); + if (!this.isDestroyed()) { + return this.errorHandler(agg); + } + return; + } if (!this.isDestroyed()) { this.successHandler(); } } catch (e) { if (!this.isDestroyed()) { this.errorHandler(e); } } }src/elements/content-explorer/MetadataSidePanel.tsx (1)
25-37: Import the error type or define the callback, fool.ErrorCallback isn’t in scope. Define it locally from ElementsXhrError or import the proper type.
Apply this diff:
import type { Selection } from 'react-aria-components'; import type { BoxItem, Collection } from '../../common/types/core'; import type { MetadataTemplate } from '../../common/types/metadata'; +import type { ElementsXhrError } from '../../common/types/api'; +type ErrorCallback = (e: ElementsXhrError) => void;Optional consistency: use JSONPatchOperations from common API types in both files.
-import { - AutofillContextProvider, - FormValues, - JSONPatchOperations, - MetadataInstance, - MetadataInstanceForm, - type MetadataTemplateField, -} from '@box/metadata-editor'; +import { + AutofillContextProvider, + FormValues, + MetadataInstance, + MetadataInstanceForm, + type MetadataTemplateField, +} from '@box/metadata-editor'; +import type { JSONPatchOperations } from '../../common/types/api';src/elements/content-explorer/MetadataQueryAPIHelper.ts (3)
186-223: Null-proof your lookups, fool — don’t crash on missing metadata.Directly accessing item.metadata[scope][templateKey] and oldField.value will blow up for items lacking that template or newly added fields.
Apply this diff:
- generateOperations = ( + generateOperations = ( item: BoxItem, templateOldFields: MetadataTemplateField[], templateNewFields: MetadataTemplateField[], ): JSONPatchOperations => { - const { scope, templateKey } = this.metadataTemplate; - const itemFields = item.metadata[scope][templateKey]; + const { scope, templateKey } = this.metadataTemplate; + const itemFields = (getProp(item, ['metadata', scope, templateKey], {}) as Record<string, any>) || {}; const operations = templateNewFields.flatMap(newField => { let newFieldValue = newField.value; const { key, type } = newField; // when retrieve value from float type field, it gives a string instead if (type === 'float' && newFieldValue !== '') { newFieldValue = Number(newFieldValue); } - const oldField = templateOldFields.find(f => f.key === key); - const oldFieldValue = oldField.value; + const oldField = templateOldFields.find(f => f.key === key); + const oldFieldValue = oldField ? oldField.value : undefined; ... return this.createJSONPatchOperations( key, shouldUseItemFieldValue ? itemFields[key] : oldFieldValue, newFieldValue, ); }); return operations; };
260-269: Make failures reject and skip empty ops, sucka.Wrap the callback API in a Promise that rejects on error, and no-op when there’s nothing to patch.
Apply this diff:
- ): Promise<void> => { - return this.api - .getMetadataAPI(true) - .updateMetadata(item, this.metadataTemplate, operations, successCallback, errorCallback); - }; + ): Promise<void> => { + if (!operations || operations.length === 0) { + if (successCallback) successCallback(); + return Promise.resolve(); + } + return new Promise<void>((resolve, reject) => { + this.api + .getMetadataAPI(true) + .updateMetadata( + item, + this.metadataTemplate, + operations, + () => { + if (successCallback) successCallback(); + resolve(); + }, + err => { + if (errorCallback) errorCallback(err); + reject(err); + }, + ); + }); + };
271-286: Per-item ops type is wrong — you built a 2D array, fool.Use JSONPatchOperations[] and short-circuit when all are empty.
Apply this diff:
- ): Promise<void> => { - const operations: JSONPatchOperations = []; - items.forEach(item => { - const operation = this.generateOperations(item, templateOldFields, templateNewFields); - operations.push(operation); - }); - return this.api - .getMetadataAPI(true) - .bulkUpdateMetadata(items, this.metadataTemplate, operations, successCallback, errorCallback); - }; + ): Promise<void> => { + const perItemOps: JSONPatchOperations[] = items.map(item => + this.generateOperations(item, templateOldFields, templateNewFields), + ); + if (perItemOps.every(ops => !ops || ops.length === 0)) { + if (successCallback) successCallback(); + return Promise.resolve(); + } + return this.api + .getMetadataAPI(true) + .bulkUpdateMetadata(items, this.metadataTemplate, perItemOps, successCallback, errorCallback); + };src/elements/content-explorer/utils.ts (3)
43-66: Fix the NaN check — don’t lie to TypeScript, fool.Guard with typeof before Number.isNaN to avoid false negatives and TS errors.
Apply this diff:
- // float - if (Number.isNaN(value)) { + // float + if (typeof value === 'number' && Number.isNaN(value)) { return true; }
129-131: Null-proof selection and metadata access, sucka.Selected items can be empty and metadata paths can be missing. Optional-chain it.
Apply this diff:
- const firstSelectedItem = selectedItems[0]; - const firstSelectedItemFieldValue = firstSelectedItem.metadata[scope][templateKey][key]; + const firstSelectedItem = selectedItems[0]; + const firstSelectedItemFieldValue = firstSelectedItem?.metadata?.[scope]?.[templateKey]?.[key]; ... - const allItemsHaveSameInitialValue = selectedItems.every(selectedItem => - areFieldValuesEqual(selectedItem.metadata[scope][templateKey][key], firstSelectedItemFieldValue), - ); + const allItemsHaveSameInitialValue = selectedItems.every(selectedItem => + areFieldValuesEqual( + selectedItem?.metadata?.[scope]?.[templateKey]?.[key], + firstSelectedItemFieldValue, + ), + );Also applies to: 141-143
156-163: Don’t mutate template options in place. Clone it, fool.Pushing into options mutates source template across renders.
Apply this diff:
- if (fieldType === 'multiSelect' || fieldType === 'enum') { - fieldValue = fieldType === 'enum' ? MULTI_VALUE_DEFAULT_VALUE : [MULTI_VALUE_DEFAULT_VALUE]; - const multiValueOption = options?.find(option => option.key === MULTI_VALUE_DEFAULT_VALUE); - if (!multiValueOption) { - options?.push(MULTI_VALUE_DEFAULT_OPTION); - } - } + if (fieldType === 'multiSelect' || fieldType === 'enum') { + fieldValue = fieldType === 'enum' ? MULTI_VALUE_DEFAULT_VALUE : [MULTI_VALUE_DEFAULT_VALUE]; + const hasMultiValue = options?.some(opt => opt.key === MULTI_VALUE_DEFAULT_VALUE); + if (!hasMultiValue) { + options = options ? [...options, MULTI_VALUE_DEFAULT_OPTION] : [MULTI_VALUE_DEFAULT_OPTION]; + } else if (options) { + options = [...options]; + } + }
🧹 Nitpick comments (4)
src/api/Metadata.js (2)
118-131: Fix the docs, fool — this is a folder URL, not files.The JSDoc still says “base url for files” while this helper targets folders. Tighten it up.
Apply this diff:
- * @param {string} id - a Box folder id - * @param {string} field - metadata field - * @return {string} base url for files + * @param {string} id - a Box folder id + * @param {string} field - metadata field + * @return {string} base url for folders
854-866: Use constants, not magic strings, sucka.Compare against TYPE_FILE for consistency and future-proofing.
Apply this diff:
- const { id, permissions, type } = item; + const { id, permissions, type } = item; ... - url: - type === 'file' + url: + type === TYPE_FILE ? this.getMetadataUrl(id, template.scope, template.templateKey) : this.getMetadataUrlForFolder(id, template.scope, template.templateKey), ... - id: type === 'file' ? getTypedFileId(id) : getTypedFolderId(id), + id: type === TYPE_FILE ? getTypedFileId(id) : getTypedFolderId(id),Also applies to: 867-877
src/elements/content-explorer/MetadataSidePanel.tsx (1)
143-144: Don’t pass null for callbacks.Use undefined to satisfy stricter types and intent.
Apply this diff:
- onChange={null} + onChange={undefined}src/elements/content-explorer/utils.ts (1)
93-100: Nit: comment says “Set” but function is getFieldValue.Call it what it is.
Apply this diff:
-// Set the field value in Metadata Form based on the field type +// Get the field value in Metadata Form based on the field type
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP (Model Context Protocol) integration is disabled
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
i18n/en-US.propertiesis excluded by!i18n/**
📒 Files selected for processing (10)
src/api/Metadata.js(5 hunks)src/api/__tests__/Metadata.test.js(4 hunks)src/elements/common/messages.js(2 hunks)src/elements/content-explorer/ContentExplorer.tsx(4 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(6 hunks)src/elements/content-explorer/MetadataSidePanel.tsx(5 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(2 hunks)src/elements/content-explorer/__tests__/MetadataSidePanel.test.tsx(3 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)src/elements/content-explorer/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
- src/elements/content-explorer/tests/MetadataSidePanel.test.tsx
- src/elements/common/messages.js
- src/api/tests/Metadata.test.js
- src/elements/content-explorer/ContentExplorer.tsx
- src/elements/content-explorer/tests/MetadataQueryAPIHelper.test.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-27T15:25:53.190Z
Learnt from: jpan-box
PR: box/box-ui-elements#4248
File: src/elements/content-explorer/MetadataViewContainer.tsx:30-44
Timestamp: 2025-08-27T15:25:53.190Z
Learning: In the Box API, there is an inconsistency where some endpoints require trimmed metadata field names (e.g., "industry") while others require the full field path (e.g., "metadata.enterprise_1515946.templateName.industry"). The trimMetadataFieldPrefix helper function in MetadataViewContainer.tsx correctly trims the field names for API endpoints that expect the shorter format.
Applied to files:
src/elements/content-explorer/MetadataQueryAPIHelper.ts
📚 Learning: 2025-08-25T16:19:22.007Z
Learnt from: greg-in-a-box
PR: box/box-ui-elements#4235
File: src/elements/content-explorer/MetadataQueryBuilder.ts:103-110
Timestamp: 2025-08-25T16:19:22.007Z
Learning: In src/elements/content-explorer/MetadataQueryBuilder.ts, the getSelectFilter function has a special case that maps 'mimetype-filter' to 'item.extension' because mimetype-filter is a multi-select field but the actual database field is 'item.extension'. This mapping is necessary and should not be removed.
Applied to files:
src/elements/content-explorer/MetadataQueryAPIHelper.tssrc/elements/content-explorer/utils.ts
🧬 Code graph analysis (3)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
src/elements/content-explorer/utils.ts (3)
areFieldValuesEqual(69-80)isEmptyValue(45-66)isMultiValuesField(102-110)src/features/metadata-based-view/MetadataQueryAPIHelper.js (1)
clonedFields(235-235)
src/elements/content-explorer/utils.ts (1)
src/elements/common/messages.js (1)
messages(9-1133)
src/elements/content-explorer/MetadataSidePanel.tsx (2)
src/elements/content-explorer/utils.ts (1)
useTemplateInstance(113-195)src/elements/common/messages.js (1)
messages(9-1133)
🪛 Biome (2.1.2)
src/api/Metadata.js
[error] 125-125: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 125-125: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 125-125: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 125-125: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 125-125: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 125-125: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 839-839: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 840-840: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 841-841: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 842-842: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 843-843: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 844-844: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 844-844: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 845-845: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 854-854: Shouldn't redeclare 'id'. Consider to delete it or rename it.
'id' is defined here:
(lint/suspicious/noRedeclare)
[error] 854-854: Shouldn't redeclare 'type'. Consider to delete it or rename it.
'type' is defined here:
(lint/suspicious/noRedeclare)
[error] 915-915: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 916-916: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 917-917: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 918-918: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 919-919: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 920-920: return 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 (2)
src/api/Metadata.js (1)
839-852: Per-item callback suppression is solid.Good pattern to avoid double-firing callbacks in bulk ops. I like it.
src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
309-311: Good call adding FIELD_PERMISSIONS.Prevents “can’t edit” surprises down the line.
4 participants
View Mode when multiple items are selected
Edit Mode when multiple items are selected
Success Notification Banner
Error Notification Banner
Summary by CodeRabbit
New Features
Accessibility
Tests