feat(metadata-view): pass-thru onSortChange #4248
Conversation
WalkthroughI pity the fool who misses this: adds client-side caching for metadata template schemas and updates metadata sorting plumbing — forwards onSortChange, introduces dual internal/user sort callbacks, widens sortBy typing to accept Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CE as ContentExplorer
participant C as Content
participant MVC as MetadataViewContainer
participant MV as MetadataView (Table)
Note over CE,MVC: Sorting flow (dual callbacks)
CE->>C: Render with onSortChange prop
C->>MVC: onSortChange forwarded
MVC->>MV: tableProps with wrapped onSortChange
MV-->>MVC: onSortChange({column, direction})
rect rgba(233,246,255,0.6)
note right of MVC: Map RAC descriptor -> internal (trimmed key, ASC/DESC)
MVC->>MVC: Call internalOnSortChange(trimmedKey, ASC|DESC)
MVC->>CE: Call consumer onSortChange(column, "ascending"/"descending") if provided
end
sequenceDiagram
autonumber
participant UI as Caller
participant M as Metadata API
participant Cache as APICache
participant XHR as XHR
Note over M: getSchemaByTemplateKey(templateKey)
UI->>M: getSchemaByTemplateKey(key)
M->>Cache: get(cacheKey)
alt Cache hit
Cache-->>M: schema
M-->>UI: schema
else Cache miss
M->>XHR: GET /metadata_templates/{key}/schema
XHR-->>M: schema
M->>Cache: set(cacheKey, schema)
M-->>UI: schema
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (4)
✨ 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 (
|
jpan-box
force-pushed
the
mdv-osc
branch
2 times, most recently
from
f51667a to
5791e15
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/elements/content-explorer/Content.tsx (1)
87-95: Avoid consumer override of internal onSortChange.Because
{...metadataViewProps}is spread afteronSortChange, a consumer-providedonSortChangeinsidemetadataViewPropswill override the internal handler, breaking pass-thru. Spread first, then setonSortChange(or omitonSortChangefrom the type).Apply:
<MetadataViewContainer currentCollection={currentCollection} isLoading={percentLoaded !== 100} hasError={view === VIEW_ERROR} metadataTemplate={metadataTemplate} - onSortChange={onSortChange} {...metadataViewProps} + onSortChange={onSortChange} />Optionally tighten the prop type:
- metadataViewProps?: Omit<MetadataViewContainerProps, 'currentCollection'>; + metadataViewProps?: Omit<MetadataViewContainerProps, 'currentCollection' | 'onSortChange'>;
🧹 Nitpick comments (9)
src/api/Metadata.js (1)
93-102: Cache key helper looks good, but consider scope collisions.If multiple scopes are ever introduced for template schemas,
templateKey-only keys could collide. Consider includingscopein the key for future-proofing.Example:
- getMetadataTemplateSchemaCacheKey(templateKey: string): string { - return `${CACHE_PREFIX_METADATA}template_schema_${templateKey}`; - } + getMetadataTemplateSchemaCacheKey(templateKey: string, scope: string = 'enterprise'): string { + return `${CACHE_PREFIX_METADATA}template_schema_${scope}_${templateKey}`; + }src/elements/content-explorer/ContentExplorer.tsx (2)
13-14: Type import alignment with RAC.Good move importing
Keyfromreact-aria-componentshere. Keep this consistent across files (see MetadataViewContainer.tsx which importsKeyfrom@react-types/shared).
155-156: Prop typing widened correctly; update State for consistency.
sortBy?: SortBy | Keyis fine. Mirror this inState.sortByto avoid narrowing back tostring.Apply:
- sortBy: SortBy | string; + sortBy: SortBy | Key;src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (2)
146-153: Strengthen assertion to verify sort actually applied.After clicking the header, assert row order changes to catch regressions where
order_by.field_keyis wrong or mocks don’t sort.Example:
- await userEvent.click(industryHeader); + await userEvent.click(industryHeader); + const rows = await canvas.findAllByRole('row'); + expect(rows[1]).toHaveAccessibleName(/Child 1/i); // or whatever the sorted first row should be
252-267: Mock handler: be lenient on direction casing and support both asc/desc.Backend expects
ASC/DESC, but callers may pass lowercase in some paths. Handle both to make the story resilient.Apply:
- const orderByDirection = body.order_by[0].direction; + const orderByDirection = String(body.order_by?.[0]?.direction || '').toUpperCase(); const orderByFieldKey = body.order_by[0].field_key; // Hardcoded case for sorting by industry - if (orderByFieldKey === `${metadataFieldNamePrefix}.industry` && orderByDirection === 'ASC') { + if (orderByFieldKey === `${metadataFieldNamePrefix}.industry` && orderByDirection === 'ASC') { const sortedMetadata = orderBy( mockMetadata.entries, 'metadata.enterprise_0.templateName.industry', 'asc', ); return HttpResponse.json({ ...mockMetadata, entries: sortedMetadata }); }Optionally add the DESC case as well for completeness.
src/elements/content-explorer/MetadataViewContainer.tsx (2)
4-4: Unify Key type source.Import
Keyfromreact-aria-components(same as ContentExplorer) to keep types consistent and avoid dual sources.Apply:
-import { type Key } from '@react-types/shared'; +import type { Key } from 'react-aria-components';
6-6: Type-only import for SortDescriptor.Import as a type to avoid bundling a non-existent runtime symbol.
Apply:
-import { SortDescriptor } from 'react-aria-components'; +import type { SortDescriptor } from 'react-aria-components';src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (2)
457-460: Verify prop name and coverage: allowsSorting
- Please confirm the prop is indeed
allowsSorting(notallowSorting) for the table column API used by Metadata View V2.- Consider adding a quick test that clicks the Name column header to verify both callbacks fire for a non-metadata column as well.
Also applies to: 465-468
509-541: Strengthen the dual-callback test to reduce flakiness and broaden assertionsGood coverage of the dual-callback contract. Two small upgrades will make this test more robust and aligned with the PR goal of “trimmed column in API”:
- Wrap callback assertions in
waitForto avoid timing flakiness after the click.- Click the header a second time to verify toggle to descending for both callback shapes.
- Avoid hard-coding the full column ID; build it from
metadataFieldNamePrefixto keep the test resilient.- Optionally, assert the POST payload for the metadata query uses the trimmed field key for sorting (either
sort_byororder_by, depending on the implementation).Suggested inline diff for this test block:
await userEvent.click(industryHeader); - // Internal callback gets trimmed version for API calls - expect(mockOnSortChangeBUIE).toHaveBeenCalledWith('industry', 'ASC'); - - // User callback gets full column ID with direction - expect(mockOnSortChangeConsumer).toHaveBeenCalledWith({ - column: 'metadata.enterprise_0.templateName.industry', - direction: 'ascending', - }); + // Internal callback gets trimmed version for API calls + await waitFor(() => { + expect(mockOnSortChangeBUIE).toHaveBeenCalledWith('industry', 'ASC'); + }); + + // User callback gets full column ID with direction + await waitFor(() => { + expect(mockOnSortChangeConsumer).toHaveBeenCalledWith({ + column: `${metadataFieldNamePrefix}.industry`, + direction: 'ascending', + }); + }); + + // Toggle to descending on second click + await userEvent.click(industryHeader); + await waitFor(() => { + expect(mockOnSortChangeBUIE).toHaveBeenCalledWith('industry', 'DESC'); + expect(mockOnSortChangeConsumer).toHaveBeenCalledWith({ + column: `${metadataFieldNamePrefix}.industry`, + direction: 'descending', + }); + });Optional: import the Xhr mock and assert the trimmed field key is sent to the API (update to
order_byif that’s what the API expects):// Add near the imports (top of file) import Xhr from '../../../utils/Xhr'; // Inside the test, after the second click: const XhrMock = Xhr as unknown as jest.Mock; const lastInstance = XhrMock.mock.instances.at(-1); const postCalls = lastInstance.post.mock.calls as Array<[ { url: string; data?: any } ]>; const lastExecuteRead = postCalls .map(args => args[0]) .filter(arg => arg.url === 'https://api.box.com/2.0/metadata_queries/execute_read') .at(-1); expect(lastExecuteRead?.data).toEqual( expect.objectContaining({ // If implementation uses `order_by`, update below accordingly sort_by: [ expect.objectContaining({ field_key: 'industry', }), ], }), );
📜 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(2 hunks)src/elements/content-explorer/Content.tsx(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(3 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(4 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
src/features/content-explorer/item-list/ItemList.js (1)
tableProps(201-201)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
src/constants.js (2)
DEFAULT_HOSTNAME_API(210-210)DEFAULT_HOSTNAME_API(210-210)
🪛 Biome (2.1.2)
src/api/Metadata.js
[error] 99-99: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 99-99: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 350-350: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 350-350: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 351-351: 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). (1)
- GitHub Check: Summary
🔇 Additional comments (4)
src/api/Metadata.js (1)
99-101: Biome “TS-only syntax” warnings are false positives for Flow types in this file.This file consistently uses Flow annotations. Ensure the formatter/linter is configured for Flow or the path is excluded from Biome’s TS parser to avoid noise.
Also applies to: 350-351
src/elements/content-explorer/ContentExplorer.tsx (1)
898-907: LGTM: sort handler accepts RAC Key.Signature and refresh logic look correct for both folder and metadata views.
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
8-9: LGTM: orderBy inclusion for mock sorting.src/elements/content-explorer/MetadataViewContainer.tsx (1)
71-77: Public onSortChange passthrough is well-scoped.Prop shape and docstring make intent clear.
jfox-box
left a comment
jfox-box
left a comment
There was a problem hiding this comment.
Overall LGTM, some comments
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
Outdated
Show resolved
Hide resolved
from the element to the shared-feature
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx
Outdated
Show resolved
Hide resolved
jfox-box
left a comment
jfox-box
left a comment
There was a problem hiding this comment.
LGTM, pending other comments
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
160-164: Guard against undefined tableProps to prevent runtime error.
Spreading an undefined value throws. Use a nullish coalesce before spreading.- const newTableProps = { - ...tableProps, - onSortChange: handleSortChange, - }; + const newTableProps = { + ...(tableProps ?? {}), + onSortChange: handleSortChange, + };src/api/Metadata.js (1)
350-367: Bug: caching/returning raw XHR response instead of payload.
This will leak transport metadata to callers and break code expecting the schema object.- async getSchemaByTemplateKey(templateKey: string): Promise<MetadataTemplateSchemaResponse> { + async getSchemaByTemplateKey(templateKey: string): Promise<MetadataTemplateSchemaResponse> { const cache: APICache = this.getCache(); const key = this.getMetadataTemplateSchemaCacheKey(templateKey); // Return cached value if it exists if (cache.has(key)) { return cache.get(key); } // Fetch from API if not cached const url = this.getMetadataTemplateSchemaUrl(templateKey); - const response = await this.xhr.get({ url }); + const { data } = await this.xhr.get({ url }); // Cache the response - cache.set(key, response); + cache.set(key, data); - return response; + return data; }
🧹 Nitpick comments (3)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
134-159: Dual-callback wrapper is correct; minor comment drift.
- Call order (internal → external) and direction mapping are right.
- Nit: the comment says “API accepts asc/desc” but the code intentionally passes “ASC”/“DESC”. Align the comment to avoid confusion.
- // API accepts asc/desc "https://developer.box.com/reference/post-metadata-queries-execute-read/" + // Upstream expects "ASC"/"DESC" here (API ultimately normalizes).src/api/Metadata.js (1)
350-367: Optional: add a forceFetch to bypass cache when needed.
Helps invalidate stale schemas without touching cache internals.- async getSchemaByTemplateKey(templateKey: string): Promise<MetadataTemplateSchemaResponse> { + async getSchemaByTemplateKey(templateKey: string, forceFetch?: boolean = false): Promise<MetadataTemplateSchemaResponse> { const cache: APICache = this.getCache(); const key = this.getMetadataTemplateSchemaCacheKey(templateKey); - if (cache.has(key)) { + if (!forceFetch && cache.has(key)) { return cache.get(key); } const url = this.getMetadataTemplateSchemaUrl(templateKey); - const { data } = await this.xhr.get({ url }); + const { data } = await this.xhr.get({ url }); cache.set(key, data); return data; }src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
509-541: Great coverage of dual onSortChange paths; consider asserting call order.
Ensures internal fires before external.await userEvent.click(industryHeader); // Internal callback gets trimmed version for API calls expect(mockOnSortChangeBUIE).toHaveBeenCalledWith('industry', 'ASC'); // User callback gets full column ID with direction expect(mockOnSortChangeConsumer).toHaveBeenCalledWith({ column: 'metadata.enterprise_0.templateName.industry', direction: 'ascending', }); + // Verify call order: internal before external + expect(mockOnSortChangeBUIE.mock.invocationCallOrder[0]) + .toBeLessThan(mockOnSortChangeConsumer.mock.invocationCallOrder[0]);
📜 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(2 hunks)src/elements/content-explorer/Content.tsx(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(3 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(4 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/elements/content-explorer/Content.tsx
- src/elements/content-explorer/ContentExplorer.tsx
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 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/MetadataViewContainer.tsx
🧬 Code graph analysis (2)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
src/features/content-explorer/item-list/ItemList.js (1)
tableProps(201-201)
src/api/Metadata.js (1)
src/constants.js (2)
CACHE_PREFIX_METADATA(63-63)CACHE_PREFIX_METADATA(63-63)
🪛 Biome (2.1.2)
src/api/Metadata.js
[error] 99-99: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 99-99: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 350-350: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 350-350: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 351-351: 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 (7)
src/elements/content-explorer/MetadataViewContainer.tsx (5)
4-6: Imports look correct.
Type additions for Key and SortDescriptor align with the new handler signature.
30-44: Prefix-trim helper is appropriate here.
Using the retrieved learning about API inconsistencies, trimming to the field key for the internal path is valid. Keep this localized to places that explicitly require trimmed keys.
75-77: Public prop surface is clear.
Prop doc clarifies internal usage; signature matches downstream expectations.
84-84: Good aliasing to onSortChangeInternal.
Improves readability alongside onSortChangeExternal.
167-174: Pass-through props look good.
items/columns/actionBar/tableProps wired as expected.src/api/Metadata.js (1)
93-102: Cache key helper LGTM.
Prefix and shape are consistent with existing cache keys.src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
457-457: Use of allowsSorting is correct.
Matches RAC prop name across columns.Also applies to: 465-465
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
509-541: Dual onSortChange test is solid—tighten it up with call counts and payload verificationLock this down so regressions don’t sneak in. I pity the fool who lets flaky sorts slide!
- Assert single invocations for both callbacks.
- Verify the MQ payload used the trimmed field key ('industry').
- Optional: add a caching test to ensure rapid clicks don’t refetch the schema.
Apply within this test:
@@ // Internal callback gets trimmed version for API calls expect(mockOnSortChangeInternal).toHaveBeenCalledWith('industry', 'ASC'); + expect(mockOnSortChangeInternal).toHaveBeenCalledTimes(1); @@ // User callback gets full column ID with direction expect(mockOnSortChangeExternal).toHaveBeenCalledWith({ column: 'metadata.enterprise_0.templateName.industry', direction: 'ascending', }); + expect(mockOnSortChangeExternal).toHaveBeenCalledTimes(1); + + // Verify the Metadata Query payload used the trimmed field key + const xhrCtor = (Xhr as unknown as jest.Mock); + const xhrInstance = xhrCtor.mock.instances[0]; + const postMock = xhrInstance.post as jest.Mock; + const mqArg = postMock.mock.calls.find(([arg]) => + arg?.url === 'https://api.box.com/2.0/metadata_queries/execute_read', + )?.[0]; + expect(mqArg?.data?.sort_by?.[0]?.field_key).toBe('industry');Add this import at the top of the file:
import Xhr from '../../../utils/Xhr';Optional caching test addition:
test('should not refetch template schema on rapid header clicks (cached)', async () => { renderComponent(metadataViewV2ElementProps); const industryHeader = await screen.findByRole('columnheader', { name: 'Industry' }); await userEvent.click(industryHeader); await userEvent.click(industryHeader); await userEvent.click(industryHeader); const xhrCtor = (Xhr as unknown as jest.Mock); const xhrInstance = xhrCtor.mock.instances[0]; const getMock = xhrInstance.get as jest.Mock; const schemaCalls = getMock.mock.calls.filter(([arg]) => arg?.url === 'https://api.box.com/2.0/metadata_templates/enterprise/templateName/schema', ); expect(schemaCalls.length).toBe(1); });
📜 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 (1)
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (1)
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
457-457: allowsSorting rename wired correctlyNice alignment with the V2 props on both name and metadata columns. Clean and clear, sucka.
Also applies to: 465-465
4 participants
from the element to the shared-feature
Desired User Behavior:
Previous State:
New State:
** handleSortChange trims the column ID before passing the ID to the API.
Verification:
Summary by CodeRabbit
New Features
Performance Improvements
Tests