feat(metadata-view): Add Filtering #4235
Conversation
WalkthroughI pity the fool who misses this: external metadata filtering is added and wired through Metadata View V2, a MetadataQueryBuilder DSL and query-merge logic are introduced, FIELD_NAME is renamed to FIELD_ITEM_NAME across helpers/tests, Content/ContentExplorer/MetadataViewContainer APIs and tests/stories/mocks are updated, and @box/metadata-view is bumped. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant C as Content
participant MVC as MetadataViewContainer
participant CE as ContentExplorer
participant MQH as MetadataQueryAPIHelperV2
participant API as Metadata API
U->>C: interact with Metadata View V2
C->>MVC: render with onMetadataFilter prop
U->>MVC: change/submit filters
MVC-->>C: invoke onMetadataFilter(ExternalFilterValues)
C-->>CE: propagate onMetadataFilter
CE->>CE: filterMetadata() sets state.metadataFilters
CE->>MQH: fetchMetadataQueryResults(query, success, error, metadataFilters)
MQH->>MQH: verifyQueryFields + build/merge filter query
MQH->>API: request with merged query + params
API-->>MQH: response (collection)
MQH-->>CE: success(collection, metadataTemplate)
CE->>U: updated collection rendered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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 (
|
greg-in-a-box
force-pushed
the
mq-helper
branch
from
739c68d to
246ca43
Compare
greg-in-a-box
force-pushed
the
mq-helper
branch
2 times, most recently
from
0709147 to
74f2a8a
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 (3)
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (3)
174-189: Fix mock lifecycle: top-level jest.fn mocks leak state across testsThe file-scoped mocks (getSchemaByTemplateKeyFunc, queryMetadataFunc, updateMetadataFunc, api) are initialized once and reused. Several tests assert “not called,” which will be brittle if another prior test already invoked the mock. Initialize these mocks per test in beforeEach (and clear in afterEach) to guarantee isolation.
Apply this diff to move declarations to mutable bindings and remove one-time initializations here:
-const getSchemaByTemplateKeyFunc = jest.fn().mockReturnValueOnce(Promise.resolve(templateSchemaResponse)); -const queryMetadataFunc = jest.fn().mockReturnValueOnce(Promise.resolve(metadataQueryResponse)); -const updateMetadataFunc = jest.fn().mockReturnValueOnce(Promise.resolve(metadataInstance)); -const api = { - getMetadataAPI: () => { - return { - getSchemaByTemplateKey: getSchemaByTemplateKeyFunc, - updateMetadata: updateMetadataFunc, - }; - }, - getMetadataQueryAPI: () => { - return { - queryMetadata: queryMetadataFunc, - }; - }, -}; +let getSchemaByTemplateKeyFunc: jest.Mock; +let queryMetadataFunc: jest.Mock; +let updateMetadataFunc: jest.Mock; +let api: { + getMetadataAPI: () => { getSchemaByTemplateKey: typeof getSchemaByTemplateKeyFunc; updateMetadata: typeof updateMetadataFunc }; + getMetadataQueryAPI: () => { queryMetadata: typeof queryMetadataFunc }; +};
333-339: Assert that filters/options are actually applied when fetchingfetchMetadataQueryResults now accepts an options/filters argument, but this test only asserts the happy path of the promise chain. Add an assertion that verifyQueryFields (or buildMDQueryParams) is called with the provided filters to catch regressions where options are ignored.
Apply this diff to add a focused test within this describe block:
test('should fetch metadata query results, template info, and call successCallback with data with data types', async () => { @@ expect(errorCallback).not.toHaveBeenCalled(); }); + + test('should forward filters to verifyQueryFields before querying', async () => { + const successCallback = jest.fn(); + const errorCallback = jest.fn(); + const filters = { + 'metadata.enterprise_1234.templateKey.status': { fieldType: 'enum', value: ['active'] }, + }; + const verifySpy = jest.spyOn(metadataQueryAPIHelper, 'verifyQueryFields'); + metadataQueryAPIHelper.queryMetadata = jest.fn().mockResolvedValue(metadataQueryResponse); + metadataQueryAPIHelper.getTemplateSchemaInfo = jest.fn().mockResolvedValue(template); + metadataQueryAPIHelper.getDataWithTypes = jest.fn().mockReturnValue(dataWithTypes); + + await metadataQueryAPIHelper.fetchMetadataQueryResults(mdQuery, filters, successCallback, errorCallback); + + expect(verifySpy).toHaveBeenCalledWith(mdQuery, filters); + expect(successCallback).toHaveBeenCalledWith(dataWithTypes, template); + expect(errorCallback).not.toHaveBeenCalled(); + });
203-214: Refactor tests to initialize mocks per test and reset after eachThe current test declares its mock functions (
getSchemaByTemplateKeyFunc,queryMetadataFunc,updateMetadataFunc) andapionce at load time usingjest.fn().mockReturnValueOnce(...). Because the repo’s Jest config enablesclearMocksandrestoreMocksbut notresetMocks, only call counts and spies are cleared between tests—not mock implementations nor one-time return values. To avoid “once-only” returns and state bleeding across multiple tests, move mock instantiation intobeforeEachand add a teardown to fully reset mocks.• In src/elements/content-explorer/tests/MetadataQueryAPIHelper.test.ts:
- Change the top-level
constdeclarations forgetSchemaByTemplateKeyFunc,queryMetadataFunc,updateMetadataFunc, andapitolet.- Inside the existing
beforeEach(() => { … }), assign each to a freshjest.fn().mockResolvedValue(…).- Instantiate
metadataQueryAPIHelperafter setting up the newapi.- Add an
afterEach(() => jest.resetAllMocks());to restore mock implementations.
• No change needed in jest.config.js (it already hasclearMocks: true/restoreMocks: true), but if you prefer configuration-driven resets you can enableresetMocks: true.Example diff:
--- a/src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts +++ b/src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts @@ top of file -let getSchemaByTemplateKeyFunc = jest.fn().mockReturnValueOnce(Promise.resolve(templateSchemaResponse)); -let queryMetadataFunc = jest.fn().mockReturnValueOnce(Promise.resolve(metadataQueryResponse)); -let updateMetadataFunc = jest.fn().mockReturnValueOnce(Promise.resolve(metadataInstance)); -let api = { … }; +let getSchemaByTemplateKeyFunc; +let queryMetadataFunc; +let updateMetadataFunc; +let api; @@ describe('elements/content-explorer/MetadataQueryAPIHelper', () => { - beforeEach(() => { - metadataQueryAPIHelper = new MetadataQueryAPIHelper(api); - metadataQueryAPIHelper.templateKey = templateKey; - metadataQueryAPIHelper.templateScope = templateScope; - metadataQueryAPIHelper.metadataTemplate = template; - metadataQueryAPIHelper.metadataQuery = mdQuery; - }); + beforeEach(() => { + // Fresh mocks each test + getSchemaByTemplateKeyFunc = jest.fn().mockResolvedValue(templateSchemaResponse); + queryMetadataFunc = jest.fn().mockResolvedValue(metadataQueryResponse); + updateMetadataFunc = jest.fn().mockResolvedValue(metadataInstance); + + api = { + getMetadataAPI: () => ({ getSchemaByTemplateKey: getSchemaByTemplateKeyFunc, updateMetadata: updateMetadataFunc }), + getMetadataQueryAPI: () => ({ queryMetadata: queryMetadataFunc }), + }; + + metadataQueryAPIHelper = new MetadataQueryAPIHelper(api); + metadataQueryAPIHelper.templateKey = templateKey; + metadataQueryAPIHelper.templateScope = templateScope; + metadataQueryAPIHelper.metadataTemplate= template; + metadataQueryAPIHelper.metadataQuery = mdQuery; + }); + + afterEach(() => { + // Reset mocks to their original implementations + jest.resetAllMocks(); + });
🧹 Nitpick comments (15)
src/elements/content-explorer/MetadataQueryBuilder.ts (3)
1-1: Consider simplifying the regex pattern for better readability.The regex pattern is correct but could be more readable by extracting the range pattern separately or adding a comment explaining the pattern structure.
-const EXTRACT_COMPARISON_FILTER_REGEXP = /^(>|<)([-\s]*[0-9]+)|\[[0-9\-,]+\]/; +// Matches comparison operators (>, <) with numbers or range notation [1,2,3] +const EXTRACT_COMPARISON_FILTER_REGEXP = /^(>|<)([-\s]*[0-9]+)|\[[0-9\-,]+\]/;
21-23: Consider improving null safety of parseNumberComparisonFilter.The function returns
RegExpMatchArray | null, but this return type isn't clearly indicated in the function signature. Consider adding an explicit return type annotation for better type safety.-export const parseNumberComparisonFilter = (value: string) => { +export const parseNumberComparisonFilter = (value: string): RegExpMatchArray | null => { return value.match(EXTRACT_COMPARISON_FILTER_REGEXP); };
119-119: Hard-coded field name transformation for 'mimetype-filter'.The special case for
'mimetype-filter'field is hard-coded. Consider extracting this as a constant or configuration to improve maintainability.+const MIMETYPE_FILTER_FIELD = 'mimetype-filter'; +const MIMETYPE_FILTER_TARGET = 'item.extension'; + export const getSelectFilter = (filterValue: string[], fieldKey: string, argIndexStart: number): QueryResult => { // ... existing code ... return { queryParams: multiSelectQueryParams, queries: [ - `(${fieldKey === 'mimetype-filter' ? 'item.extension' : fieldKey} HASANY (${Object.keys( + `(${fieldKey === MIMETYPE_FILTER_FIELD ? MIMETYPE_FILTER_TARGET : fieldKey} HASANY (${Object.keys( multiSelectQueryParams, )src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts (2)
294-294: Type assertion issue in test.Passing
nulltogetSelectFilterwhich expectsstring[]requires a type assertion but could mask type safety issues. Consider testing withundefinedas well or using a more explicit type guard in the implementation.- it('should return empty result for null/undefined', () => { - const result = getSelectFilter(null, 'field_name', 0); + it('should return empty result for null/undefined', () => { + const result = getSelectFilter(null as any, 'field_name', 0); expect(result).toEqual({ queryParams: {}, queries: [], keysGenerated: 0, }); + + const resultUndefined = getSelectFilter(undefined as any, 'field_name', 0); + expect(resultUndefined).toEqual({ + queryParams: {}, + queries: [], + keysGenerated: 0, + }); });
350-357: Duplicate test for null/undefined in getMimeTypeFilter.Similar to the getSelectFilter test, this also tests null input which requires type assertion.
- it('should return empty result for null/undefined', () => { - const result = getMimeTypeFilter(null, 'mimetype', 0); + it('should return empty result for null/undefined', () => { + const result = getMimeTypeFilter(null as any, 'mimetype', 0); expect(result).toEqual({ queryParams: {}, queries: [], keysGenerated: 0, }); + + const resultUndefined = getMimeTypeFilter(undefined as any, 'mimetype', 0); + expect(resultUndefined).toEqual({ + queryParams: {}, + queries: [], + keysGenerated: 0, + }); });src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
222-337: Consider refactoring the switch statement for better maintainability.The
buildMDQueryParamsmethod has a complex switch statement that could benefit from refactoring. Consider extracting each case into separate helper methods for better readability and testability.buildMDQueryParams = (filters: ExternalFilterValues) => { let argIndex = 0; - const generateArgKeyFromFieldKey = (key: string) => { - argIndex += 1; - return generateArgKey(key, argIndex); - }; - let queries: string[] = []; let queryParams: { [key: string]: number | Date | string } = {}; if (filters) { - let argKey1; - let argKey2; - Object.keys(filters).forEach(key => { const filter = filters[key]; if (!filter) { return; } const { fieldType, value } = filter; + const result = this.processFilterByType(key, fieldType, value, argIndex); + if (result) { + queryParams = mergeQueryParams(queryParams, result.queryParams); + queries = mergeQueries(queries, result.queries); + argIndex += result.keysGenerated; + } - switch (fieldType) { - // ... existing switch cases - } }); } const query = queries.reduce((acc, curr, index) => { if (index > 0) { acc += ` AND (${curr})`; } else { acc = `(${curr})`; } return acc; }, ''); return { queryParams, query, }; }; + +private processFilterByType( + key: string, + fieldType: string, + value: any, + argIndex: number +): { queryParams: any; queries: string[]; keysGenerated: number } | null { + // Move switch case logic here +}
273-275: Improve error handling for invalid JSON parsing.Using
console.errorfor error handling is not ideal. Consider propagating the error or using a more robust error handling mechanism.} catch { - console.error('Invalid filter number range'); + // Log error with more context + console.error(`Invalid filter number range for key "${key}": ${filterValue}`); + // Consider propagating error or returning error state }src/elements/content-explorer/MetadataViewContainer.tsx (1)
68-71: Consider removing the redundant wrapper function.The
transformInternalFieldsToPublicfunction is just a wrapper aroundconvertFilterValuesToExternal. Consider usingconvertFilterValuesToExternaldirectly in the component.-// Internal helper function for component use -function transformInternalFieldsToPublic(fields: FilterValues): ExternalFilterValues { - return convertFilterValuesToExternal(fields); -}Then update Line 117 to use
convertFilterValuesToExternaldirectly:-const transformed = transformInternalFieldsToPublic(fields); +const transformed = convertFilterValuesToExternal(fields);src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (7)
229-235: Avoid brittle index-based assertion in flattenMetadata testAsserting on fields[0] depends on ordering. Prefer selecting the field by key to make the test resilient.
Apply this diff:
- expect(result.enterprise.fields).toHaveLength(3); - expect(result.enterprise.fields[0].type).toBeUndefined(); + expect(result.enterprise.fields).toHaveLength(3); + const typeField = result.enterprise.fields.find( + f => f.key === `${FIELD_METADATA}.${templateScope}.${templateKey}.type`, + ); + expect(typeField?.type).toBeUndefined();
287-299: “not.toHaveBeenCalled” assertions require isolated mocksThese tests assert that getSchemaByTemplateKeyFunc was not called. Without the mock isolation change above, prior tests may have incremented call counts, causing false failures. The beforeEach refactor will fix this. If you prefer not to refactor, call getSchemaByTemplateKeyFunc.mockClear() before the assertions.
358-367: Good error propagation coverage for fetchMetadataQueryResultsThe query failure path is asserted cleanly. Consider adding a test for a thrown (synchronous) error inside getDataWithTypes to ensure errorCallback is invoked in that scenario, too.
615-883: Reduce coupling to generated arg key names; add a couple more filter casesA lot of assertions depend on the exact arg key names (e.g., _1, _2). That’s fine if we want a stable contract, but it makes tests noisy for innocuous internal refactors (like changing key generators). Consider asserting with expect.objectContaining for queryParams and using regex/toContain for query fragments. Also, add coverage for:
- string field with multiple search terms (OR of ILIKEs)
- malformed range string (non-JSON) fallback behavior
Apply this diff to add a multi-term string test:
test('should handle string field with search value', () => { @@ expect(result.queryParams.arg_metadata_enterprise_12345_template_name_1).toBe('%search term%'); }); + + test('should handle string field with multiple search values using OR ILIKE', () => { + const filters = { + 'metadata.enterprise_12345.template.name': { + fieldType: 'string', + value: ['alpha', 'beta'], + }, + }; + const result = metadataQueryAPIHelper.buildMDQueryParams(filters); + expect(result.queries).toHaveLength(1); + expect(result.queries[0]).toContain('ILIKE'); + expect(result.queries[0]).toContain('OR'); + expect(result.queryParams.arg_metadata_enterprise_12345_template_name_1).toBe('%alpha%'); + expect(result.queryParams.arg_metadata_enterprise_12345_template_name_2).toBe('%beta%'); + });Optionally, relax arg key assertions like:
- expect(result.queryParams.arg_metadata_enterprise_12345_template_year_1).toBe(2020); + expect(Object.values(result.queryParams)).toEqual(expect.arrayContaining([2020]));
885-906: Relax overly strict string equality on generated queryAsserting the full query string including parentheses makes the test fragile against harmless formatting changes. Assert on the essential fragment and params instead.
Apply this diff:
- expect(result.query).toBe( - '((metadata.enterprise_1234.templateKey.status HASANY (:arg_metadata_enterprise_1234_templateKey_status_1)))', - ); + expect(result.query).toContain( + 'metadata.enterprise_1234.templateKey.status HASANY', + );
749-761: Consider covering exact-match string (no wildcard) vs contains behavior explicitlyThe current test ensures ILIKE with wrapped wildcards. If exact-match semantics are ever supported, a parallel test would catch regressions. If contains-only is the intended contract, it’s fine to keep as-is.
768-776: mimetype test is great; add one for uppercase/extension normalization if supportedIf the builder normalizes extensions (e.g., PDF → pdf), add a case to lock that in.
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
package.json(1 hunks)src/elements/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(10 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(4 hunks)src/elements/content-explorer/MetadataQueryBuilder.ts(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(4 hunks)src/elements/content-explorer/__tests__/Content.test.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(10 hunks)src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
PR: box/box-ui-elements#4224
File: package.json:296-297
Timestamp: 2025-08-12T18:04:17.698Z
Learning: In the box-ui-elements project, the team is comfortable with raising peerDependency minimum versions when upgrading blueprint-web packages, even if it's a breaking change for consumers.
Applied to files:
package.json
🧬 Code Graph Analysis (7)
src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts (1)
src/elements/content-explorer/MetadataQueryBuilder.ts (9)
parseNumberComparisonFilter(21-23)mergeQueryParams(25-30)mergeQueries(32-34)generateArgKey(36-39)escapeValue(41-41)getStringFilter(43-52)getRangeFilter(54-96)getSelectFilter(98-127)getMimeTypeFilter(129-156)
src/elements/content-explorer/Content.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
ExternalFilterValues(17-24)
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
ContentExplorerProps(106-164)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (3)
MetadataViewContainerProps(73-78)ExternalFilterValues(17-24)convertFilterValuesToExternal(48-66)
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js (8)
metadataQueryAPIHelper(15-15)metadataInstanceId1(19-19)metadataQueryResponse(70-116)nextMarker(69-69)getSchemaByTemplateKeyFunc(191-191)queryMetadataFunc(192-192)mdQuery(207-218)updateMetadataFunc(193-193)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
ExternalFilterValues(17-24)src/elements/content-explorer/MetadataQueryBuilder.ts (8)
generateArgKey(36-39)getRangeFilter(54-96)mergeQueryParams(25-30)mergeQueries(32-34)parseNumberComparisonFilter(21-23)getMimeTypeFilter(129-156)getSelectFilter(98-127)getStringFilter(43-52)
src/elements/content-explorer/ContentExplorer.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
MetadataViewContainerProps(73-78)ExternalFilterValues(17-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (31)
package.json (1)
139-139: LGTM! DevDependency update aligns with feature requirements.The update of
@box/metadata-viewfrom^0.41.2to^0.42.2is appropriate for adding the filtering functionality.src/elements/content-explorer/MetadataQueryBuilder.ts (1)
41-41: LGTM! Proper escaping for SQL LIKE wildcards.The escaping of
_and%characters is correctly implemented to prevent SQL injection and ensure proper LIKE pattern matching.src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts (1)
14-50: Comprehensive test coverage for parseNumberComparisonFilter.The tests thoroughly cover various scenarios including edge cases with negative numbers and invalid inputs.
src/elements/content-explorer/__tests__/Content.test.tsx (1)
33-33: LGTM! Mock properly updated for new API.The addition of
onMetadataFilter: jest.fn()correctly aligns with the new filtering functionality.src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (2)
471-478: LGTM! Test properly updated for metadata filtering.The test correctly includes the new
onMetadataFiltermock and properly types the props.
500-500: Good removal of unnecessary async wrapper.Removing the
waitForwrapper for a synchronous assertion improves test clarity.src/elements/content-explorer/Content.tsx (3)
7-7: LGTM! Clean type import for filtering functionality.The import of
ExternalFilterValuesis properly structured and maintains clear separation of concerns by importing both the default export and the type from the same module.
41-41: LGTM! Well-defined required prop for metadata filtering.The
onMetadataFilterprop is correctly typed as a required callback that acceptsExternalFilterValues. This ensures proper data flow from the UI to the parent component.
61-61: LGTM! Proper destructuring and forwarding of the filter callback.The
onMetadataFilterprop is correctly destructured from props and forwarded toMetadataViewContainer, maintaining clean prop drilling.Also applies to: 94-94
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (4)
6-10: LGTM! Well-structured test imports for filtering functionality.The imports are properly organized, bringing in both the component and its public exports (
convertFilterValuesToExternalandExternalFilterValues) for comprehensive testing.
21-21: Good fix: Corrected typo in test data.Fixed the stray space in the key value from
' name'to'name', ensuring test data integrity.
35-50: LGTM! Enhanced test coverage with additional field types.Good addition of
price(float) andcategory(multiSelect) fields to the test template, providing broader coverage for the filter conversion logic.
380-503: Excellent test coverage forconvertFilterValuesToExternal!The test suite comprehensively covers:
- Enum to string array conversion
- Range value preservation
- Float value handling
- Mixed field types
- Edge cases (empty objects, empty arrays)
- MultiSelect value preservation
This provides robust validation of the conversion logic.
src/elements/content-explorer/MetadataQueryAPIHelper.ts (3)
30-40: LGTM! Well-organized imports for metadata query building.The imports from
MetadataQueryBuilderare comprehensive and provide all necessary utilities for filter construction.
193-197: LGTM! Clean API enhancement for external filtering.The addition of the
fieldsparameter tofetchMetadataQueryResultsproperly extends the API to support external filtering while maintaining backward compatibility.
362-371: LGTM! Well-implemented filter integration.The
verifyQueryFieldsmethod correctly integrates external filters by:
- Building filter query and params from external filters
- Merging with existing custom query using AND operator
- Updating the metadataQuery object appropriately
The implementation maintains backward compatibility by making the
fieldsparameter optional.src/elements/content-explorer/MetadataViewContainer.tsx (4)
17-24: LGTM! Well-structured external filter type definition.The
ExternalFilterValuestype is well-designed, providing a flexible structure that supports various field types while maintaining type safety through the use of union types for options and fieldType.
48-66: LGTM! Clean conversion logic from internal to external format.The
convertFilterValuesToExternalfunction correctly handles the transformation of enum values to string arrays while preserving range/float objects as-is. Good use of type assertions and proper typing.
115-124: LGTM! Proper callback composition.The
handleFilterSubmitcallback correctly:
- Transforms internal fields to external format
- Calls the required
onMetadataFilterprop- Conditionally calls
onFilterSubmitif providedThis ensures both the parent component and any additional handlers receive the properly formatted data.
98-99: LGTM! Clean filter chip configuration.Good changes to the filter configuration:
- Using just the field key for the ID (removing suffix) improves clarity
- Adding
shouldRenderChip: trueensures proper UI renderingAlso applies to: 102-102
src/elements/content-explorer/ContentExplorer.tsx (4)
184-184: LGTM! Proper state initialization for metadata filters.The
metadataFiltersstate is correctly initialized as an empty object with the properExternalFilterValuestype.Also applies to: 310-310
1702-1704: LGTM! Clean filter update handler.The
filterMetadatamethod correctly updates state and triggers collection refresh, providing a clean interface for metadata filtering.
481-486: LGTM! Proper integration of filters into query execution.The metadata query flow correctly passes
metadataFilterstofetchMetadataQueryResultsfor both V2 and maintains backward compatibility for V1 by not passing filters.Also applies to: 495-499
1857-1857: LGTM! Proper wiring of filter callback to Content component.The
onMetadataFilterprop is correctly passed to the Content component, completing the data flow from UI to state management.src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (7)
13-13: Describe path rename looks correct and matches the new locationThe suite name aligns with the moved helper under elements/content-explorer.
314-321: Good negative-path coverage for queryMetadataThe rejection path is exercised properly, and the expectation uses rejects.toThrow with the original error.
406-435: Nice additions for string and array JSON Patch casesThese tests ensure full-replace semantics for arrays and verify string replacements. This guards against subtle regression in patch generation.
443-466: Solid coverage for getMetadataQueryFields edge casesScenarios with no metadata fields, empty arrays, and undefined fields are covered well.
497-523: updateMetadata error path exercised properlyMocking updateMetadata to call errorCallback validates the bubble-up behavior.
599-613: verifyQueryFields: robust handling of non-array fieldsGood guardrail to coerce fields to an array and enforce required fields.
885-952: Filters + verifyQueryFields end-to-end looks goodThese tests validate AND composition and required fields injection. With the mock isolation fix, they should be stable.
greg-in-a-box
force-pushed
the
mq-helper
branch
from
74f2a8a to
c0629ce
Compare
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/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
273-273: Fix invalid JSON parsing test expectation.The test at lines 840-856 for handling invalid range JSON expects the parsing to succeed with 5 elements in the array
[1,2,3,4,5]. However, the comment says it should "only use the first two elements for the range", but the actual implementation would parse this as a valid JSON array and use elements 0 and 1 (values 1 and 2), not handle it as an error case.test('should handle invalid range JSON gracefully', () => { const filters = { 'metadata.enterprise_12345.template.year': { fieldType: 'float', - value: '[1,2,3,4,5]', + value: 'not-valid-json', }, }; const result = metadataQueryAPIHelper.buildMDQueryParams(filters); - // The value is valid JSON but has more than 2 elements, so it should still work - // but only use the first two elements for the range - expect(result.queries).toHaveLength(1); - expect(Object.keys(result.queryParams)).toHaveLength(2); - expect(result.queries[0]).toContain('>='); - expect(result.queries[0]).toContain('<='); + // Invalid JSON should be caught and logged, no query generated + expect(result.queries).toHaveLength(0); + expect(Object.keys(result.queryParams)).toHaveLength(0); });
♻️ Duplicate comments (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
138-141: Fix the incorrect prop exclusion syntax.The
Omittype utility has incorrect syntax. There's a missing closing quote aftercurrentCollection, which causes the entire string to be treated as a single property name instead of excluding multiple properties.-metadataViewProps?: Omit< - MetadataViewContainerProps, - 'hasError' | 'currentCollection | metadataTemplate | onMetadataFilter' ->; +metadataViewProps?: Omit< + MetadataViewContainerProps, + 'hasError' | 'currentCollection' | 'metadataTemplate' | 'onMetadataFilter' +>;
🧹 Nitpick comments (11)
src/elements/content-explorer/__tests__/Content.test.tsx (5)
41-44: Instrument the MetadataViewContainer mock to capture and trigger onMetadataFilterThe current mock renders static markup, so we can’t assert that Content forwards onMetadataFilter or that it’s invoked. Replace the mock with a lightweight, clickable test double that calls onMetadataFilter. This will let us add a behavior test under the v2 flag.
Apply this diff to replace the mock:
- jest.mock('../MetadataViewContainer', () => ({ - __esModule: true, - default: () => <div>MetadataViewContainer</div>, - })); + const MockMetadataViewContainer = jest.fn(({ onMetadataFilter }: { onMetadataFilter?: (v: unknown) => void }) => ( + <button data-testid="metadata-view-container" onClick={() => onMetadataFilter?.({ foo: 'bar' })}> + MetadataViewContainer + </button> + )); + + jest.mock('../MetadataViewContainer', () => ({ + __esModule: true, + default: MockMetadataViewContainer, + }));Follow-up test to add (see next comment for where to place it).
116-125: Add a test that verifies onMetadataFilter is forwarded and invoked under v2With the instrumented mock, add a focused test to ensure Content passes the callback through and that it fires when the mock triggers it. This guards the main behavior introduced by the PR.
Also import fireEvent to support the click:
- import { render, screen } from '../../../test-utils/testing-library'; + import { render, screen, fireEvent } from '../../../test-utils/testing-library';Then add this test inside the metadataViewV2 describe block:
test('forwards onMetadataFilter to MetadataViewContainer and invokes it', () => { const onMetadataFilter = jest.fn(); renderComponent({ currentCollection: collection, features, fieldsToShow: ['id'], view: VIEW_METADATA, onMetadataFilter, }); // Ensure new view renders const mvc = screen.getByTestId('metadata-view-container'); expect(mvc).toBeInTheDocument(); // Simulate filter submit inside the MVC fireEvent.click(mvc); expect(onMetadataFilter).toHaveBeenCalledWith(expect.any(Object)); });
77-81: Reduce brittleness in date text assertionAsserting an exact string like “Viewed Oct 10, 2023” can be locale/format dependent and flaky. Prefer a regex that tolerates minor formatting differences, or assert the presence of the “Viewed ” prefix and the date separately.
Example tweak:
- expect(screen.getByText('Viewed Oct 10, 2023')).toBeInTheDocument(); + expect(screen.getByText(/Viewed .*10, 2023/)).toBeInTheDocument();
88-91: Apply the same robustness to the grid-mode date assertionMirror the regex approach here to avoid format-related flakes.
- expect(screen.getByText('Viewed Oct 10, 2023')).toBeInTheDocument(); + expect(screen.getByText(/Viewed .*10, 2023/)).toBeInTheDocument();
86-86: Use renderComponent for consistencyEverywhere else you use the renderComponent helper; switch this instance to the helper for consistency and to ensure the same wrappers/providers apply.
- render(<Content {...mockProps} viewMode={VIEW_MODE_GRID} currentCollection={collection} />); + renderComponent({ viewMode: VIEW_MODE_GRID, currentCollection: collection });src/elements/content-explorer/MetadataViewContainer.tsx (4)
2-9: Consider optimizing imports.Some types like
RangeTypeare imported individually when they might be more maintainable if grouped with related imports from the same module.-import { - EnumType, - FloatType, - MetadataFormFieldValue, - MetadataTemplateFieldOption, - RangeType, -} from '@box/metadata-filter'; +import type { + EnumType, + FloatType, + MetadataFormFieldValue, + MetadataTemplateFieldOption, + RangeType, +} from '@box/metadata-filter';
14-15: Consider a more descriptive type name.The type alias
EnumToStringArraycould be more clearly named to indicate its purpose in transforming enum types to string arrays.-type EnumToStringArray<T> = T extends EnumType ? string[] : T; -type ExternalMetadataFormFieldValue = EnumToStringArray<MetadataFormFieldValue>; +type ConvertEnumToStringArray<T> = T extends EnumType ? string[] : T; +type ExternalMetadataFormFieldValue = ConvertEnumToStringArray<MetadataFormFieldValue>;
68-71: Consider removing unnecessary wrapper function.The
transformInternalFieldsToPublicfunction is just a thin wrapper aroundconvertFilterValuesToExternalwith no additional logic. Consider usingconvertFilterValuesToExternaldirectly.-// Internal helper function for component use -function transformInternalFieldsToPublic(fields: FilterValues): ExternalFilterValues { - return convertFilterValuesToExternal(fields); -}Then update line 117 to call
convertFilterValuesToExternaldirectly:-const transformed = transformInternalFieldsToPublic(fields); +const transformed = convertFilterValuesToExternal(fields);
98-98: Consider adding default value for shouldRenderChip.The
shouldRenderChipproperty is hardcoded totruefor all filter chips. Consider making this configurable if different fields might need different chip rendering behavior in the future.-id: `${field.key}`, +id: field.key, name: field.displayName, fieldType: field.type, options: field.options?.map(({ key }) => key) || [], -shouldRenderChip: true, +shouldRenderChip: field.shouldRenderChip ?? true,src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
324-336: Query reduction logic could be more readable.The reduce function building the final query string could be simplified for better readability.
-const query = queries.reduce((acc, curr, index) => { - if (index > 0) { - acc += ` AND (${curr})`; - } else { - acc = `(${curr})`; - } - return acc; -}, ''); +const query = queries.length > 0 + ? queries.map(q => `(${q})`).join(' AND ') + : '';
191-206: Update JSDoc to document the new fields parameter.The
fetchMetadataQueryResultsmethod signature has changed to include afieldsparameter, but the JSDoc comments haven't been updated.Add JSDoc for the new parameter:
+/** + * Fetches metadata query results with optional filters + * @param {MetadataQueryType} metadataQuery - The metadata query to execute + * @param {ExternalFilterValues} fields - Optional external filter values to apply + * @param {SuccessCallback} successCallback - Callback for successful results + * @param {ErrorCallback} errorCallback - Callback for errors + * @return {Promise<void>} + */ fetchMetadataQueryResults = (
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
package.json(1 hunks)src/elements/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(10 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(4 hunks)src/elements/content-explorer/MetadataQueryBuilder.ts(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(4 hunks)src/elements/content-explorer/__tests__/Content.test.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(10 hunks)src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/elements/content-explorer/tests/MetadataQueryBuilder.test.ts
- src/elements/content-explorer/MetadataQueryBuilder.ts
- src/elements/content-explorer/Content.tsx
- src/elements/content-explorer/tests/MetadataViewContainer.test.tsx
- package.json
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
ExternalFilterValues(17-24)src/elements/content-explorer/MetadataQueryBuilder.ts (8)
generateArgKey(36-39)getRangeFilter(54-96)mergeQueryParams(25-30)mergeQueries(32-34)parseNumberComparisonFilter(21-23)getMimeTypeFilter(129-156)getSelectFilter(98-127)getStringFilter(43-52)
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js (7)
metadataInstanceId1(19-19)metadataQueryResponse(70-116)nextMarker(69-69)getSchemaByTemplateKeyFunc(191-191)queryMetadataFunc(192-192)mdQuery(207-218)updateMetadataFunc(193-193)
src/elements/content-explorer/ContentExplorer.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
MetadataViewContainerProps(73-78)ExternalFilterValues(17-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (11)
src/elements/content-explorer/__tests__/Content.test.tsx (1)
33-33: LGTM: Added onMetadataFilter to mock propsThis aligns the test fixture with the new ContentProps surface. No issues with this line.
src/elements/content-explorer/MetadataViewContainer.tsx (2)
48-66: LGTM! Clean implementation of filter value conversion.The
convertFilterValuesToExternalfunction properly handles the transformation of internal filter values to the external format, correctly converting enum types to string arrays while preserving range and float types.
115-124: LGTM! Well-structured filter submission handler.The
handleFilterSubmitcallback properly transforms internal values to external format and calls both the requiredonMetadataFilterand optionalonFilterSubmitcallbacks in the correct order.src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
362-371: LGTM! Clean integration of filter parameters.The
verifyQueryFieldsmethod properly integrates the external filters into the metadata query, correctly merging the filter query with existing custom queries and combining query parameters.src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (3)
229-250: LGTM! Good edge case coverage for flattenMetadata.The new test cases effectively cover edge scenarios including empty template fields and missing field values, ensuring the function handles incomplete data gracefully.
314-321: LGTM! Proper error handling test.The test correctly verifies that API errors are properly propagated through the promise rejection chain.
615-883: Comprehensive test coverage for buildMDQueryParams.Excellent test coverage for the
buildMDQueryParamsmethod, covering all filter types, edge cases, null/undefined values, empty arrays, and invalid JSON scenarios. The tests are well-structured and thoroughly verify the query building logic.src/elements/content-explorer/ContentExplorer.tsx (4)
96-96: Good type imports organization.The addition of
ExternalFilterValuesto the import fromMetadataViewContaineris clean and follows the existing pattern.
480-487: LGTM! Clean integration of metadata filters.The implementation correctly passes
metadataFiltersto the V2 API helper while maintaining backward compatibility with the V1 path. The conditional logic based on the feature flag is well-structured.Also applies to: 495-500
1702-1704: LGTM! Simple and effective filter implementation.The
filterMetadatamethod correctly updates state and triggers a collection refresh when filters change.
394-395: Good JSDoc update for the new parameter.The JSDoc comment has been properly updated to document the new
metadataTemplateparameter.
greg-in-a-box
force-pushed
the
mq-helper
branch
from
c0629ce to
16957e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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)
82-83: Bug: No error fallback UI when metadataViewV2 is enabledEmptyView is suppressed entirely when the v2 feature flag is on, so VIEW_ERROR shows no error state. Render EmptyView for errors regardless of the flag.
- {!isMetadataViewV2Feature && isViewEmpty && <EmptyView view={view} isLoading={percentLoaded !== 100} />} + {(view === VIEW_ERROR || (!isMetadataViewV2Feature && isViewEmpty)) && ( + <EmptyView view={view} isLoading={view === VIEW_ERROR ? false : percentLoaded !== 100} /> + )}
♻️ Duplicate comments (1)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
222-337: Harden buildMDQueryParams: null guards, string handling, and simplified logicThere are correctness and robustness issues:
- string case assumes value[0], breaking when value is a string
- date/float case lacks null/undefined guard, risking NaN params
- JSON range parsing quietly logs and continues (okay), but we should avoid generating broken params
Apply the minimal, targeted fix below.
buildMDQueryParams = (filters: ExternalFilterValues) => { let argIndex = 0; const generateArgKeyFromFieldKey = (key: string) => { argIndex += 1; return generateArgKey(key, argIndex); }; let queries: string[] = []; let queryParams: { [key: string]: number | Date | string } = {}; if (filters) { - let argKey1; - let argKey2; - Object.keys(filters).forEach(key => { const filter = filters[key]; if (!filter) { return; } - const { fieldType, value } = filter; + const { fieldType, value } = filter; + // Skip null/undefined values + if (value == null) { + return; + } switch (fieldType) { case 'date': case 'float': { if (typeof value === 'object' && value !== null && 'range' in value) { const result = getRangeFilter(value, key, argIndex); queryParams = mergeQueryParams(queryParams, result.queryParams); queries = mergeQueries(queries, result.queries); argIndex += result.keysGenerated; break; } const numberOperators = parseNumberComparisonFilter(String(value)); if (numberOperators && numberOperators.length > 2) { const filterType: string = numberOperators[1]; const filterValue: string = filterType ? numberOperators[2] : numberOperators[0]; if (['>', '<'].includes(filterType)) { - argKey1 = generateArgKeyFromFieldKey(key); - queryParams[argKey1] = parseFloat(filterValue); - queries.push(`(${key} ${filterType} :${argKey1})`); + const argKey = generateArgKeyFromFieldKey(key); + queryParams[argKey] = parseFloat(filterValue); + queries.push(`(${key} ${filterType} :${argKey})`); } else { // filter by range try { const valueRange = JSON.parse(filterValue); - argKey1 = generateArgKeyFromFieldKey(key); - argKey2 = generateArgKeyFromFieldKey(key); - queryParams[argKey1] = valueRange[0]; - queryParams[argKey2] = valueRange[1]; - queries.push(`(${key} >= :${argKey1} AND ${key} <= :${argKey2})`); + if (Array.isArray(valueRange) && valueRange.length >= 2) { + const argKey1 = generateArgKeyFromFieldKey(key); + const argKey2 = generateArgKeyFromFieldKey(key); + queryParams[argKey1] = valueRange[0]; + queryParams[argKey2] = valueRange[1]; + queries.push(`(${key} >= :${argKey1} AND ${key} <= :${argKey2})`); + } else { + // Ignore invalid range shape + } } catch { console.error('Invalid filter number range'); } } } else { - argKey1 = generateArgKeyFromFieldKey(key); - queryParams[argKey1] = parseFloat(String(value)); - queries.push(`(${key} = :${argKey1})`); + const num = parseFloat(String(value)); + if (!Number.isNaN(num)) { + const argKey = generateArgKeyFromFieldKey(key); + queryParams[argKey] = num; + queries.push(`(${key} = :${argKey})`); + } } break; } case 'enum': case 'multiSelect': { const arrayValue = Array.isArray(value) ? value.map(v => String(v)) : [String(value)]; let result; if (key === 'mimetype-filter') { result = getMimeTypeFilter(arrayValue, key, argIndex); } else { result = getSelectFilter(arrayValue, key, argIndex); } queryParams = mergeQueryParams(queryParams, result.queryParams); queries = mergeQueries(queries, result.queries); argIndex += result.keysGenerated; break; } case 'string': { - if (value && value[0]) { - const result = getStringFilter(value[0], key, argIndex); + const stringValue = Array.isArray(value) ? value[0] : value; + if (stringValue) { + const result = getStringFilter(String(stringValue), key, argIndex); queryParams = mergeQueryParams(queryParams, result.queryParams); queries = mergeQueries(queries, result.queries); argIndex += result.keysGenerated; } break; } default: { if (Array.isArray(value)) { const args = value.map(v => { const qpKey = generateArgKeyFromFieldKey(key); queryParams[qpKey] = v; return `:${qpKey}`; }); queries.push(`(${key} IN (${args.join(', ')}))`); } break; } } }); } const query = queries.reduce((acc, curr, index) => { if (index > 0) { acc += ` AND ${curr}`; } else { acc = curr; } return acc; }, ''); return { queryParams, query, }; }
🧹 Nitpick comments (8)
src/elements/content-explorer/__tests__/Content.test.tsx (1)
41-44: Strengthen assertion: verify prop forwarding to MetadataViewContainerRight now the mock swallows props, so we don't assert that onMetadataFilter is actually forwarded. Consider capturing props in the mock and asserting the callback is passed through in the v2 path.
jest.mock('../MetadataViewContainer', () => ({ __esModule: true, - default: () => <div>MetadataViewContainer</div>, + default: (props: any) => ( + <div data-testid="metadata-view-container" data-has-on-metadata-filter={!!props.onMetadataFilter}> + MetadataViewContainer + </div> + ), }));Then add an assertion in the v2 test:
expect(screen.getByText('MetadataViewContainer')).toBeInTheDocument(); +expect(screen.getByTestId('metadata-view-container')).toHaveAttribute('data-has-on-metadata-filter', 'true');src/elements/content-explorer/Content.tsx (2)
91-100: hasError is always false for MetadataViewContainerThe container only renders when view === VIEW_METADATA, so hasError={view === VIEW_ERROR} is always false. Either remove the prop or rework error handling (e.g., render the container with hasError in error states if that’s intended).
Would you like me to adjust this to pass a meaningful hasError, or remove the prop entirely if the container has its own internal error UI?
7-7: Avoid coupling a UI type to a component fileImporting ExternalFilterValues from MetadataViewContainer creates needless coupling. Consider moving shared types to a standalone types module to prevent accidental circular deps and to improve reuse.
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (3)
803-819: Test name does not match scenario“should handle invalid range JSON gracefully” uses an object range, not invalid JSON. Recommend renaming for clarity or add an explicit invalid-JSON case exercising the catch branch.
Example rename:
- “should handle numeric range object with both bounds”
And optionally add:- invalid JSON string range (e.g., value: '=[1, 2, 3') to assert it’s ignored without throwing.
713-724: Add a test for string filter value passed as a plain stringYou cover string filters as array ['search term']; also cover value: 'search term' to ensure buildMDQueryParams handles both shapes.
Example:
const result = metadataQueryAPIHelper.buildMDQueryParams({ name: { fieldType: 'string', value: 'search term' }, }); expect(result.query).toBe('(name ILIKE :arg_name_1)'); expect(result.queryParams.arg_name_1).toBe('%search term%');
847-871: Add merge test when custom query and params already existverifyQueryFields merges filterQuery with an existing custom query and its query_params. Add a test with an initial metadataQuery having both query and query_params to assert proper AND merge and parameter union without collisions.
src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
339-348: Nit: preserve operator precedence when merging query fragmentsWrap fragments in parentheses to avoid surprises when customQuery contains ORs.
- return `${customQuery} AND ${filterQuery}`; + return `(${customQuery}) AND (${filterQuery})`;src/elements/content-explorer/ContentExplorer.tsx (1)
1702-1704: Optional: expose filter submissions to top-level consumersPR description mentions letting consumers create permalinks/handle submissions externally. If that means ContentExplorer’s API consumers, consider exposing an optional onMetadataFilterSubmit prop and invoking it here before refreshing.
export interface ContentExplorerProps { + onMetadataFilterSubmit?: (fields: ExternalFilterValues) => void; ... } // in filterMetadata - filterMetadata = (fields: ExternalFilterValues) => { - this.setState({ metadataFilters: fields }, this.refreshCollection); - }; + filterMetadata = (fields: ExternalFilterValues) => { + this.props.onMetadataFilterSubmit?.(fields); + this.setState({ metadataFilters: fields }, this.refreshCollection); + };
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
package.json(1 hunks)src/elements/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(10 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(4 hunks)src/elements/content-explorer/MetadataQueryBuilder.ts(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(4 hunks)src/elements/content-explorer/__tests__/Content.test.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(10 hunks)src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/elements/content-explorer/tests/MetadataQueryBuilder.test.ts
- src/elements/content-explorer/tests/MetadataViewContainer.test.tsx
- src/elements/content-explorer/MetadataQueryBuilder.ts
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
- package.json
- src/elements/content-explorer/MetadataViewContainer.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
src/elements/content-explorer/Content.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
MetadataViewContainerProps(73-78)ExternalFilterValues(17-24)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
ExternalFilterValues(17-24)src/elements/content-explorer/MetadataQueryBuilder.ts (8)
generateArgKey(36-39)getRangeFilter(54-96)mergeQueryParams(25-30)mergeQueries(32-34)parseNumberComparisonFilter(21-23)getMimeTypeFilter(129-156)getSelectFilter(98-127)getStringFilter(43-52)
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js (8)
metadataQueryAPIHelper(15-15)metadataInstanceId1(19-19)metadataQueryResponse(70-116)nextMarker(69-69)getSchemaByTemplateKeyFunc(191-191)queryMetadataFunc(192-192)mdQuery(207-218)updateMetadataFunc(193-193)
src/elements/content-explorer/ContentExplorer.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
MetadataViewContainerProps(73-78)ExternalFilterValues(17-24)
⏰ 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/__tests__/Content.test.tsx (1)
33-33: LGTM: added onMetadataFilter to mock propsAdding onMetadataFilter aligns the test with the updated ContentProps contract. No issues.
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (2)
333-339: Good coverage: v2 fetch path exercises callbacks and data typingThis validates the new fetch signature with filters and the success path wiring. Looks solid.
358-367: Good failure-path coverage for query errorsThe test ensures errors from queryMetadata are propagated to errorCallback. Nice.
src/elements/content-explorer/ContentExplorer.tsx (4)
138-141: LGTM: corrected Omit exclusion listThe Omit union is now syntactically correct and excludes the intended props.
184-185: LGTM: internal metadataFilters state wiringAdding metadataFilters to state is appropriate for v2 filtering and defaults to an empty record.
446-487: Fetch path correctly passes filters to v2 helperThe v2 call now includes metadataFilters and applies metadataView-specific ordering. Looks good.
1857-1857: LGTM: plumbing onMetadataFilter down to ContentThis wires the UI to the new filtering flow.
greg-in-a-box
force-pushed
the
mq-helper
branch
4 times, most recently
from
3e41b57 to
9d8dc73
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/elements/content-explorer/MetadataQueryBuilder.ts (2)
143-144: Safer suffix removal for Type values.Good improvement over unconditional string replacement! The code now safely checks for the 'Type' suffix before removing it.
72-72: Fix inconsistent comparison operators for range filters.The between-values logic uses
>=and<=(inclusive), but the single gt/lt cases also use inclusive operators. This inconsistency with the naming could lead to confusion -gttypically means "greater than" (exclusive), not "greater than or equal to" (inclusive).Consider using exclusive operators for consistency with standard naming conventions:
} else if (gt) { // Only gt: greater than const argKey = generateArgKey(fieldKey, (currentArgIndex += 1)); queryParams[argKey] = Number(gt); - queries.push(`(${fieldKey} >= :${argKey})`); + queries.push(`(${fieldKey} > :${argKey})`); } else if (lt) { // Only lt: less than const argKey = generateArgKey(fieldKey, (currentArgIndex += 1)); queryParams[argKey] = Number(lt); - queries.push(`(${fieldKey} <= :${argKey})`); + queries.push(`(${fieldKey} < :${argKey})`);Alternatively, if inclusive behavior is intended, update the comments to reflect this:
- // Only gt: greater than + // Only gt: greater than or equal tosrc/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
222-332: Complex filter building logic needs better error handling.The
buildMDQueryParamsmethod has several issues that need attention:
- JSON parsing at line 264 lacks error handling
- The string filter assumes value is an array at line 295
- Complex nested logic could be simplified
Apply this diff to improve error handling and simplify the logic:
} else { - const valueRange = JSON.parse(filterValue); + // filter by range + try { + const valueRange = JSON.parse(filterValue); + if (!Array.isArray(valueRange) || valueRange.length < 2) { + console.error('Invalid filter range format:', filterValue); + break; + } - argKey1 = generateArgKeyFromFieldKey(key); - argKey2 = generateArgKeyFromFieldKey(key); - queryParams[argKey1] = valueRange[0]; - queryParams[argKey2] = valueRange[1]; - queries.push(`(${key} >= :${argKey1} AND ${key} <= :${argKey2})`); + argKey1 = generateArgKeyFromFieldKey(key); + argKey2 = generateArgKeyFromFieldKey(key); + queryParams[argKey1] = valueRange[0]; + queryParams[argKey2] = valueRange[1]; + queries.push(`(${key} >= :${argKey1} AND ${key} <= :${argKey2})`); + } catch (error) { + console.error('Invalid filter number range:', error); + } }And for the string filter:
case 'string': { - if (value && value[0]) { - const result = getStringFilter(value[0], key, argIndex); + // Handle string value which could be a string or string array + const stringValue = Array.isArray(value) ? value[0] : value; + if (stringValue) { + const result = getStringFilter(stringValue, key, argIndex); queryParams = mergeQueryParams(queryParams, result.queryParams); queries = mergeQueries(queries, result.queries); argIndex += result.keysGenerated; } break; }
359-365: Fix property name mismatch when accessing query parameters.The code destructures
query_paramsfrom clonedQuery but then tries to usecustomQueryParamswhich will be undefined. Also, mergeQueryParams receives parameters in the wrong order.Apply this diff to fix the property access:
const { query: filterQuery, queryParams: filteredQueryParams } = this.buildMDQueryParams(fields); const { query: customQuery, query_params: customQueryParams } = clonedQuery; const query = this.mergeQuery(customQuery, filterQuery); - const queryParams = mergeQueryParams(filteredQueryParams, customQueryParams); + const queryParams = mergeQueryParams(customQueryParams || {}, filteredQueryParams);src/elements/content-explorer/ContentExplorer.tsx (1)
138-141: Fixed Omit syntax for excluded props — resolved.The union of string literal types is correct and matches the intended exclusions from
MetadataViewContainerProps.
🧹 Nitpick comments (4)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
68-71: Consider removing the wrapper function.The
transformInternalFieldsToPublicfunction simply delegates toconvertFilterValuesToExternalwithout adding any functionality. Consider usingconvertFilterValuesToExternaldirectly in the component.-// Internal helper function for component use -function transformInternalFieldsToPublic(fields: FilterValues): ExternalFilterValues { - return convertFilterValuesToExternal(fields); -}Then update line 117:
- const transformed = transformInternalFieldsToPublic(fields); + const transformed = convertFilterValuesToExternal(fields);src/elements/content-explorer/ContentExplorer.tsx (3)
184-185: State plumbing for external filters looks correct.
metadataFilters: ExternalFilterValuesadded to the state type and initialized to{}is consistent with theRecord<string, …>shape.- No runtime impact since it’s type-only and defaulting to empty filters.
Optional: To make intent explicit to readers, you could cast the initializer as
{} as ExternalFilterValues, but this is not required.- metadataFilters: {}, + metadataFilters: {} as ExternalFilterValues,Also applies to: 310-312
394-401: JSDoc type mismatch for metadataTemplate parameter.The function is typed with
metadataTemplate: MetadataTemplate, but JSDoc still says{Object}. Align JSDoc to improve editor/tooling hints.- * @param {Object} metadataTemplate - Metadata template object + * @param {MetadataTemplate} metadataTemplate - Metadata template object
446-458: Ensure React state immutability formarkers& confirm helper signatures
You’re correctly passing
metadataFiltersinto the V2 helper call.Two follow-ups:
Avoid mutating
markersdirectly
In showMetadataQueryResults (ContentExplorer.tsx lines 446–458), you’re doing:const { markers, currentPageNumber } = this.state; … if (currentPageNumber === 0) { markers[currentPageNumber] = metadataQueryClone.marker; }That mutates the array stored in React state without using
setState. Instead, clone it and include it in your state update:- const { currentPageNumber, markers, … } = this.state; + const { currentPageNumber, markers: prevMarkers, … } = this.state; const metadataQueryClone = cloneDeep(metadataQuery); - if (currentPageNumber === 0) { - markers[currentPageNumber] = metadataQueryClone.marker; - } + let markers = prevMarkers; + if (currentPageNumber === 0) { + markers = [...prevMarkers]; + markers[currentPageNumber] = metadataQueryClone.marker; + } … - this.setState({ - searchQuery: '', - currentCollection: this.currentUnloadedCollection(), - view: VIEW_METADATA, - }); + this.setState({ + searchQuery: '', + currentCollection: this.currentUnloadedCollection(), + view: VIEW_METADATA, + ...(currentPageNumber === 0 ? { markers } : {}), + });This preserves immutability and avoids subtle React bugs.
Helper signatures are correct as used
- V2 (TypeScript):
fetchMetadataQueryResults(metadataQuery: MetadataQueryType, fields: ExternalFilterValues, successCallback: SuccessCallback, errorCallback: ErrorCallback): Promise<void>- V1 (JavaScript):
fetchMetadataQueryResults(metadataQuery: MetadataQueryType, successCallback: SuccessCallback, errorCallback: ErrorCallback): Promise<void>
Your calls in ContentExplorer:// V2 branch this.metadataQueryAPIHelper.fetchMetadataQueryResults( metadataQueryClone, metadataFilters, this.showMetadataQueryResultsSuccessCallback, this.errorCallback, ); // V1 branch this.metadataQueryAPIHelper.fetchMetadataQueryResults( metadataQueryClone, this.showMetadataQueryResultsSuccessCallback, this.errorCallback, );match the helper definitions exactly.
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
package.json(1 hunks)src/elements/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(10 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(4 hunks)src/elements/content-explorer/MetadataQueryBuilder.ts(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(4 hunks)src/elements/content-explorer/__tests__/Content.test.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(10 hunks)src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/elements/content-explorer/tests/MetadataQueryBuilder.test.ts
- src/elements/content-explorer/tests/Content.test.tsx
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
- src/elements/content-explorer/Content.tsx
- package.json
🧰 Additional context used
🧬 Code graph analysis (4)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
ExternalFilterValues(17-24)src/elements/content-explorer/MetadataQueryBuilder.ts (8)
generateArgKey(36-39)getRangeFilter(54-96)mergeQueryParams(25-30)mergeQueries(32-34)parseNumberComparisonFilter(21-23)getMimeTypeFilter(129-157)getSelectFilter(98-127)getStringFilter(43-52)
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js (8)
metadataQueryAPIHelper(15-15)metadataInstanceId1(19-19)metadataQueryResponse(70-116)nextMarker(69-69)getSchemaByTemplateKeyFunc(191-191)queryMetadataFunc(192-192)mdQuery(207-218)updateMetadataFunc(193-193)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (3)
MetadataViewContainerProps(73-78)ExternalFilterValues(17-24)convertFilterValuesToExternal(48-66)
src/elements/content-explorer/ContentExplorer.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
MetadataViewContainerProps(73-78)ExternalFilterValues(17-24)
⏰ 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 (2)
48-66: LGTM! Clean conversion logic.The
convertFilterValuesToExternalfunction properly transforms internal filter values to the external format, correctly handling enum types by extracting the array from theenumproperty while preserving range and float objects as-is.
115-124: Well-structured filter submission handler.The
handleFilterSubmitcallback properly transforms the internal filter values and calls both handlers in the correct order, ensuring that the external consumer receives the transformed values.src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
380-503: Comprehensive test coverage for convertFilterValuesToExternal.Excellent test suite covering various scenarios including enum to string array conversion, range value preservation, mixed field types, edge cases with empty objects and arrays, and multiSelect handling. The tests are well-structured and thorough.
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (2)
615-827: Excellent test coverage for buildMDQueryParams.Comprehensive test suite covering all filter types, edge cases, null/undefined handling, and multiple filter combinations. The tests are well-organized and thorough.
829-923: Well-structured integration tests for verifyQueryFields with filters.Good coverage of filter integration with query building, parameter merging, and AND logic combination. The tests properly verify that existing query parameters are preserved when adding filters.
src/elements/content-explorer/ContentExplorer.tsx (2)
96-96: Type-only import prevents runtime cycles — good call.Using
import typeforExternalFilterValuesandMetadataViewContainerPropsavoids pullingMetadataViewContainerat runtime and eliminates circular import risks. Nothing to change.
1857-1857: WiringonMetadataFilterthrough Content — looks correct.This matches the new state handler and keeps
metadataViewPropsclean by omittingonMetadataFilterfrom consumer overrides.
greg-in-a-box
force-pushed
the
mq-helper
branch
2 times, most recently
from
56f37e8 to
277e430
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
295-296: Type assumption without validationThe code assumes
value[0]exists without checking ifvalueis an array or if it has elements. This could cause a runtime error ifvalueis not an array or is empty.Apply this fix to handle both string and array types safely:
case 'string': { - if (value && value[0]) { - const result = getStringFilter(value[0], key, argIndex); + const stringValue = Array.isArray(value) ? value[0] : value; + if (stringValue) { + const result = getStringFilter(stringValue, key, argIndex); queryParams = mergeQueryParams(queryParams, result.queryParams); queries = mergeQueries(queries, result.queries); argIndex += result.keysGenerated; } break; }
264-271: Invalid JSON parsing could crash the applicationThe code parses
filterValueas JSON without any validation or error handling. If the JSON is malformed, this will throw an unhandled exception that could crash the application.Add proper error handling:
} else { - const valueRange = JSON.parse(filterValue); + // filter by range + try { + const valueRange = JSON.parse(filterValue); + if (!Array.isArray(valueRange) || valueRange.length < 2) { + // Invalid range format - skip this filter + break; + } - argKey1 = generateArgKeyFromFieldKey(key); - argKey2 = generateArgKeyFromFieldKey(key); - queryParams[argKey1] = valueRange[0]; - queryParams[argKey2] = valueRange[1]; - queries.push(`(${key} >= :${argKey1} AND ${key} <= :${argKey2})`); + argKey1 = generateArgKeyFromFieldKey(key); + argKey2 = generateArgKeyFromFieldKey(key); + queryParams[argKey1] = valueRange[0]; + queryParams[argKey2] = valueRange[1]; + queries.push(`(${key} >= :${argKey1} AND ${key} <= :${argKey2})`); + } catch (error) { + console.error('Invalid filter number range:', error); + // Skip this filter if JSON parsing fails + } }src/elements/content-explorer/ContentExplorer.tsx (2)
138-141: Omit exclusion list fixed correctly.The union of string literal keys is now syntactically correct and achieves the intended prop exclusion.
1702-1705: Reset marker-based pagination when filters change to avoid stale pages.Applying new filters while preserving
currentPageNumber/markerscan reuse a marker from a prior query and return the wrong page/results. Reset both before refreshing.Apply this diff:
- filterMetadata = (fields: ExternalFilterValues) => { - this.setState({ metadataFilters: fields }, this.refreshCollection); - }; + filterMetadata = (fields: ExternalFilterValues) => { + this.setState( + { + metadataFilters: fields, + currentPageNumber: 0, + markers: [], + }, + this.refreshCollection, + ); + };Optional follow-up: similarly reset
currentPageNumberandmarkerswhen sort changes in metadata view so a new sort starts from a fresh first page. I can provide a patch if you want.
🧹 Nitpick comments (2)
src/elements/common/__mocks__/mockMetadata.ts (1)
15-15: Optional: Add a record withoutscoreto validate “exists/doesn’t exist” behavior.All entries now include
score. Consider leaving one withoutscoreto test “has value / is empty” cases and ensure the chip UI and query builder handle missing fields correctly.If helpful, I can propose a minimal edit to drop
scorefrom one item and add a test that asserts the “exists” filter toggles the record set as expected.Also applies to: 38-38, 60-60, 82-82, 102-102, 124-124, 143-143
src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
192-197: Update function signature documentationThe
fetchMetadataQueryResultsmethod signature has changed to acceptfields: ExternalFilterValuesas the second parameter, but the JSDoc comment above theverifyQueryFieldsmethod (lines 345-352) should be updated to reflect this new signature in the public API documentation.
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
package.json(1 hunks)src/elements/common/__mocks__/mockMetadata.ts(7 hunks)src/elements/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(10 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(4 hunks)src/elements/content-explorer/MetadataQueryBuilder.ts(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(4 hunks)src/elements/content-explorer/__tests__/Content.test.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(10 hunks)src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(6 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- package.json
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
- src/elements/content-explorer/tests/MetadataViewContainer.test.tsx
- src/elements/content-explorer/MetadataQueryBuilder.ts
- src/elements/content-explorer/tests/Content.test.tsx
- src/elements/content-explorer/Content.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (18)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
334-343: LGTM! Clean query merging logicThe
mergeQueryhelper function correctly handles all edge cases for combining custom and filter queries with AND operator.src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (2)
153-154: Consistent filter key namingGood job updating the filter keys from
industry-filterandrole-filtertoindustryandroleto align with the external filter naming convention.
207-284: Well-structured visual regression tests for data typesThe new story exports provide comprehensive coverage for different metadata field types (string, date, float) with proper assertions for both row content and sortable headers. The test structure is consistent and thorough.
src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts (1)
1-369: Comprehensive test coverage for query builder utilitiesExcellent test coverage for all the query builder functions. The tests properly cover:
- Edge cases (empty values, null/undefined inputs)
- Normal operations with various data types
- Special handling for mime types and range filters
- Proper escaping of special characters in ILIKE queries
src/elements/content-explorer/MetadataViewContainer.tsx (3)
48-66: Well-designed filter value conversion functionThe
convertFilterValuesToExternalfunction correctly transforms internal filter values to the external format, properly handling enum arrays and preserving range/float objects.
115-124: Proper callback compositionThe
handleFilterSubmitimplementation correctly chains the filter transformation and callbacks, ensuring bothonMetadataFilter(required) andonFilterSubmit(optional) are called with the transformed values.
98-103: Good UX improvement with shouldRenderChipSetting
shouldRenderChip: trueensures filter chips are always visible to users, improving discoverability of the filtering feature.src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (3)
229-250: Excellent edge case test coverageGreat additions for testing edge cases in
flattenMetadata():
- Handling empty template fields array
- Gracefully handling missing metadata fields
These tests ensure the code is robust against malformed or incomplete data.
616-827: Comprehensive filter building test coverageThe
buildMDQueryParams()tests thoroughly cover:
- All supported field types (date, float, enum, multiSelect, string)
- Edge cases (null values, empty arrays, unknown types)
- Complex scenarios (multiple filters with AND logic)
- Special handling for mime-type filters
The test coverage ensures the filtering logic works correctly across all scenarios.
829-923: Well-structured integration tests for filter verificationThe tests for
verifyQueryFieldswith filters properly validate:
- Query and query_params generation from filters
- Merging of existing queries with filter queries using AND logic
- Preservation of required fields (NAME, EXTENSION)
- Handling of cases with no filters
src/elements/content-explorer/ContentExplorer.tsx (8)
96-96: Type-only import is correct and prevents runtime cycles.Good use of
import typeto avoid bundlingMetadataViewContainerat runtime. This keeps dependencies lean and sidesteps potential circular import issues.
184-185: State addsmetadataFilterswith accurate typing.Introduction of
metadataFilters: ExternalFilterValuesis appropriate for wiring filter state through the query path.
310-311: Sane default formetadataFilters.Initializing to
{}is fine as a no-op filter set for both V1 and V2 paths.
394-394: JSDoc param addition is aligned with implementation.The success callback documents
metadataTemplateand the method stores it into state; this keeps types and docs in sync.
446-447: ReadingmetadataFiltersin the query path is correct.Pulling
metadataFiltersalongside pagination and sort state ensures a single source of truth for filter-driven queries.
495-499: V1 fetch remains unchanged, as expected.Leaving V1 untouched avoids unintended behavior changes for legacy metadata view.
1857-1857: Resolved: ContentProps and MetadataViewContainer wiring verifiedI’ve confirmed that:
- In
src/elements/content-explorer/Content.tsx, theContentPropsinterface declares
onMetadataFilter?: (fields: ExternalFilterValues) => void.- The
MetadataViewContainerPropsinterface insrc/elements/content-explorer/MetadataViewContainer.tsxrequires
onMetadataFilter: (fields: ExternalFilterValues) => void, and its implementation callsonMetadataFilter(transformed).- In
Content.tsx, theonMetadataFilterprop is correctly passed through to<MetadataViewContainer>(andmetadataViewPropsexplicitly omits it to avoid conflicts).No changes are needed.
480-486: V2 helper signature and call site alignment validated.
- Verified in
src/elements/content-explorer/MetadataQueryAPIHelper.tsthat
fetchMetadataQueryResults(metadataQuery: MetadataQueryType, fields: ExternalFilterValues, successCallback: SuccessCallback, errorCallback: ErrorCallback)
correctly accepts external filters as its second argument.- Confirmed in
src/elements/content-explorer/ContentExplorer.tsx(lines 480–486) that the callpassesthis.metadataQueryAPIHelper.fetchMetadataQueryResults( metadataQueryClone, metadataFilters, this.showMetadataQueryResultsSuccessCallback, this.errorCallback, );metadataFiltersas intended.No changes required—approving these code changes.
greg-in-a-box
force-pushed
the
mq-helper
branch
from
277e430 to
6b111bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
267-275: Handle string filters that may be a string or string[].Accessing
value[0]assumes an array. Ifvalueis a string, this passes only the first character into the query.Apply this diff:
- case 'string': { - if (value && value[0]) { - const result = getStringFilter(value[0], key, argIndex); + case 'string': { + // value can be a string or a string[] + const stringValue = Array.isArray(value) ? value[0] : value; + if (typeof stringValue === 'string' && stringValue.trim()) { + const result = getStringFilter(stringValue, key, argIndex); queryParams = mergeQueryParams(queryParams, result.queryParams); queries = mergeQueries(queries, result.queries); argIndex += result.keysGenerated; } break; }
330-339: Default query_params when merging to avoid spreading undefined.If
query_paramsis absent,{ ...undefined }will throw.Apply this diff:
- const { query: customQuery, query_params: customQueryParams } = clonedQuery; + const { query: customQuery, query_params: customQueryParams = {} } = clonedQuery; const query = this.mergeQuery(customQuery, filterQuery); const queryParams = mergeQueryParams(filteredQueryParams, customQueryParams);src/elements/content-explorer/ContentExplorer.tsx (2)
138-141: Fixed Omit exclusions — thank you for addressing this.The union of string literals is now correct, so the omitted props are actually excluded.
1702-1705: Reset marker-based pagination when filters change to avoid stale pages.Applying new
metadataFilterspreservescurrentPageNumberandmarkers, which can reuse markers from a different query and return wrong results. Reset them on filter change.Apply this diff:
- filterMetadata = (fields: ExternalFilterValues) => { - this.setState({ metadataFilters: fields }, this.refreshCollection); - }; + filterMetadata = (fields: ExternalFilterValues) => { + this.setState( + { + metadataFilters: fields, + currentPageNumber: 0, + markers: [], + }, + this.refreshCollection, + ); + };Optional: also reset when sort changes in metadata view (see comment below).
🧹 Nitpick comments (20)
package.json (1)
139-139: DevDependency bump to @box/metadata-view v0.43.x looks good; align the peerDependency to avoid version drift.Right now devDependencies use ^0.43.0, but peerDependencies still declare ^0.41.2 (Line 306). This can cause warnings or mismatched APIs for consumers. Recommend bumping the peer minimum to match.
Apply this diff to update the peer dependency:
- "@box/metadata-view": "^0.41.2", + "@box/metadata-view": "^0.43.0",Context: Per our retrieved learning, the team is comfortable raising peerDependency minimums when upgrading related packages; while this isn’t blueprint-web, aligning here reduces consumer friction.
src/elements/content-explorer/MetadataViewContainer.tsx (2)
73-78: Make onMetadataFilter optional (avoid breaking change) and default to no-op.This prop is newly introduced and required. If any internal or third-party usage renders MetadataViewContainer without it, this is a breaking change. Making it optional reduces blast radius.
Apply this diff to make it optional and provide a default:
-export interface MetadataViewContainerProps extends Omit<MetadataViewProps, 'items' | 'actionBarProps'> { +export interface MetadataViewContainerProps extends Omit<MetadataViewProps, 'items' | 'actionBarProps'> { actionBarProps?: ActionBarProps; currentCollection: Collection; metadataTemplate: MetadataTemplate; - onMetadataFilter: (fields: ExternalFilterValues) => void; + onMetadataFilter?: (fields: ExternalFilterValues) => void; }And set a default in the component signature:
-const MetadataViewContainer = ({ +const MetadataViewContainer = ({ actionBarProps, columns, currentCollection, metadataTemplate, - onMetadataFilter, + onMetadataFilter = () => {}, ...rest }: MetadataViewContainerProps) => {
115-124: Avoid double submission side-effects when both callbacks are provided.You invoke both
onMetadataFilterandonFilterSubmit. If a consumer usesonFilterSubmitto trigger fetching (or permalink updates) and you also trigger fetching viaonMetadataFilter, it can cause duplicates.Consider gating with precedence (prefer external
onMetadataFilterif provided), or pass an option flag to indicate who owns network submission. I can draft the change if you want that ownership model.src/elements/content-explorer/MetadataQueryBuilder.ts (2)
37-46: Guard against empty string values in getStringFilter to avoid query that matches everything.Currently, an empty string yields
%%which can return all rows.Apply this diff:
export const getStringFilter = (filterValue: string, fieldKey: string, argIndexStart: number): QueryResult => { let currentArgIndex = argIndexStart; - const argKey = generateArgKey(fieldKey, (currentArgIndex += 1)); + const trimmed = (filterValue ?? '').trim(); + if (!trimmed) { + return { queryParams: {}, queries: [], keysGenerated: 0 }; + } + const argKey = generateArgKey(fieldKey, (currentArgIndex += 1)); return { - queryParams: { [argKey]: `%${escapeValue(filterValue)}%` }, + queryParams: { [argKey]: `%${escapeValue(trimmed)}%` }, queries: [`(${fieldKey} ILIKE :${argKey})`], keysGenerated: currentArgIndex - argIndexStart, }; };
92-121: Minor: consider de-duplicating arg placeholders for identical values.If the same value appears multiple times, you can avoid redundant params. Not required, but it trims query length for large multi-selects.
If desired, I can propose a small memoization around
multiSelectQueryParams.src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts (2)
97-124: Add a test for empty string handling in getStringFilter.To prevent regressions where
''becomes%%, add:describe('getStringFilter', () => { + test('should return empty result for empty string', () => { + const result = getStringFilter('', 'field_name', 0); + expect(result).toEqual({ queryParams: {}, queries: [], keysGenerated: 0 }); + });
126-187: Add tests for zero-bound ranges and non-numeric guardrails.Covers 0 values and invalid numeric input.
describe('getRangeFilter', () => { + test('should include lower bound when gt is 0', () => { + const filterValue = { range: { gt: 0, lt: 10 } }; + const result = getRangeFilter(filterValue as any, 'field_name', 0); + expect(result).toEqual({ + queryParams: { arg_field_name_1: 0, arg_field_name_2: 10 }, + queries: ['(field_name >= :arg_field_name_1 AND field_name <= :arg_field_name_2)'], + keysGenerated: 2, + }); + }); + + test('should return empty result when gt is NaN', () => { + const filterValue = { range: { gt: 'x', lt: 10 } }; + const result = getRangeFilter(filterValue as any, 'field_name', 0); + expect(result).toEqual({ queryParams: {}, queries: [], keysGenerated: 0 }); + });src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
221-305: Optional: simplify query join and skip empty fragments.
reducehere can be replaced with afilter(Boolean).join(' AND ')and will naturally skip empty strings.I can send a micro-refactor if you want; functionally equivalent.
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (9)
229-235: Good coverage for empty template fields; consider asserting keys and values more explicitly.The test validates that types are undefined when the template fields are empty. For extra robustness, also assert that each expected key is still present and the values match the source metadata (besides missing type).
236-251: Graceful handling of missing template fields is well covered.Nice edge case. Consider adding a complementary case where the metadata has an extra field not present in the template (ensure it’s still surfaced consistently or intentionally ignored).
413-435: Robust edge cases forgetMetadataQueryFields().Covers no metadata fields, empty
fields, andundefinedfields. Consider adding a case with malformed metadata keys (e.g.,metadata.enterprise_1234.templateKeywithout the field) to assert defensive parsing.
558-574: Empty/nullable filters producing empty query is correct.These are important guardrails; tests look good. If possible, add a test covering
undefinedfilters key values nested inside the filters object.
575-613: Range semantics: gt/lt mapped to >=/<= — verify naming vs behavior.The tests encode inclusive bounds for
gt/ltas>=/<=. That’s fine if intentional, but the naming may confuse consumers expecting strict inequality. Consider either:
- Renaming to
gte/lte, or- Documenting that
gt/ltare inclusive.Also consider adding a test for exact equality-only ranges (e.g.,
{ range: { gt: 2020, lt: 2020 } }should likely collapse toyear = :arg).
615-681: Enum/multiselect/string/mimetype cases are well covered.
HASANYfor enum/multiselect is correct.ILIKEwith%term%ensures substring search.mimetype-filtermapping toitem.extension IN (...)is great.Consider adding:
- A string-multi-value test (e.g.,
['foo', 'bar']) to define whether the builder usesORorAND.- A case to ensure whitespace is trimmed from string inputs before building
%...%.
683-705: Multi-filter AND composition and stable arg numbering look good.Small nit: numbering order depends on object key iteration. Since insertion order is deterministic in modern JS, you’re fine, but a test asserting stability when filters are provided in a different insertion order could prevent regressions.
732-744: Unknown field type fallback to IN is reasonable.Consider logging or telemetry in the builder when falling back to help detect unexpected types in production.
772-866: Filter merge intoverifyQueryFieldsis well specified.
- Confirms AND merge with existing query and param merging.
- Ensures required fields remain present.
Consider one more test: existing query wrapped in parentheses and ending with whitespace to ensure spacing/parenthesis handling remains correct after merge.
src/elements/content-explorer/ContentExplorer.tsx (3)
184-185: State:metadataFiltersadded.Initialization pathway looks fine; ensure
ExternalFilterValuesallows{}as the empty value (e.g.,Record<string, ...>). If it’s not already typed that way, considerPartial<ExternalFilterValues>for the state property.
495-499: Mirror the local-variable pattern for the V1 path for consistency.Not strictly required, but keeps both branches consistent and avoids future typing pitfalls.
Apply this diff:
- this.metadataQueryAPIHelper = new MetadataQueryAPIHelper(this.api); - this.metadataQueryAPIHelper.fetchMetadataQueryResults( - metadataQueryClone, - this.showMetadataQueryResultsSuccessCallback, - this.errorCallback, - ); + const helperV1 = new MetadataQueryAPIHelper(this.api); + this.metadataQueryAPIHelper = helperV1; + helperV1.fetchMetadataQueryResults( + metadataQueryClone, + this.showMetadataQueryResultsSuccessCallback, + this.errorCallback, + );
907-916: Optional: reset markers when sort changes in metadata view.Sorting while in
VIEW_METADATAcurrently preserves markers. That can likewise cause stale paging on a new sort order. Consider resettingcurrentPageNumberandmarkerswhenview === VIEW_METADATA.Here’s a minimal sketch outside the diffed range:
sort = (sortBy: SortBy, sortDirection: SortDirection) => { const { view } = this.state; if (view === VIEW_METADATA) { this.setState({ sortBy, sortDirection, currentPageNumber: 0, markers: [] }, this.refreshCollection); return; } const { currentCollection: { id } } = this.state; if (id) this.setState({ sortBy, sortDirection }, this.refreshCollection); };
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
package.json(1 hunks)src/elements/common/__mocks__/mockMetadata.ts(7 hunks)src/elements/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(10 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(4 hunks)src/elements/content-explorer/MetadataQueryBuilder.ts(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(4 hunks)src/elements/content-explorer/__tests__/Content.test.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(9 hunks)src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(6 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/elements/content-explorer/Content.tsx
- src/elements/common/mocks/mockMetadata.ts
- src/elements/content-explorer/tests/Content.test.tsx
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
- src/elements/content-explorer/tests/MetadataViewContainer.test.tsx
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
PR: box/box-ui-elements#4224
File: package.json:296-297
Timestamp: 2025-08-12T18:04:17.698Z
Learning: In the box-ui-elements project, the team is comfortable with raising peerDependency minimum versions when upgrading blueprint-web packages, even if it's a breaking change for consumers.
Applied to files:
package.json
⏰ 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 (17)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
91-108: Validate options shape for filterGroups.You’re mapping template options to
string[]of keys. Ensure@box/metadata-viewexpectsoptions: string[]here (not an array of objects). If it expects objects, chips may render incorrectly or break keyboard navigation.If needed, I can adjust this to pass
{ key, displayText }objects or whatever the component expects.src/elements/content-explorer/MetadataQueryBuilder.ts (2)
30-33: LGTM: deterministic and sanitized argument keys.Replacing non-word chars with underscores avoids collisions and keeps placeholders stable across runs.
123-151: LGTM: correct Type suffix handling and IN semantics for mimetype.Suffix-stripping is now restricted to actual suffixes; the IN clause is parameterized.
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (10)
13-13: Updated describe path looks good.Suite naming now matches the file’s location under elements/content-explorer. No action needed.
261-267: Undefined template schema response handling is correct.Validates that dataWithTypes can be produced without a schema. Looks good.
314-321: API error propagation test is solid.Confirms
queryMetadatarejects on underlying API errors. No changes needed.
333-339: Updated V2 fetch signature is exercised correctly.Passing
{}for filters validates the new parameter is optional at call-sites. Good.
341-357: Error handling during template fetch is verified.The promise-chain failure path and callback usage look correct.
358-367: Query rejection path coverage is good.Ensures
errorCallbackis invoked with the rejection reason.
542-556: Non-arrayfieldshandling is validated.Ensures required fields are injected and the result is normalized to an array. Looks good.
706-731: Null/empty string inputs yield no clauses — correct.Good defensive behavior. No changes needed.
745-770: Empty arrays yield empty queries — correct.Good to avoid generating invalid
IN ()orHASANY ().
287-299: Repository-level Jest configuration already ensures isolated mocks
The project’s Jest config (scripts/jest/jest.config.js) enablesclearMocks: trueandrestoreMocks: true, so all mock call counts and implementations are automatically reset between tests. This covers the module-scopedgetSchemaByTemplateKeyFunc, preventing any cross-test leakage. No additionalbeforeEach(() => jest.clearAllMocks())or per-test mock re-initialization is required.src/elements/content-explorer/ContentExplorer.tsx (4)
96-97: Type import additions look correct.Brings in
ExternalFilterValuesandMetadataViewContainerPropsfor the new filtering flow. No changes needed.
395-400: Success callback signature expanded to receivemetadataTemplate.Aligns with V2 behavior and state updates. Looks good.
446-447: PlumbingmetadataFiltersinto the V2 fetch path is correct.Destructuring includes
metadataFilters,sortBy, andsortDirectionfor the query. Good.
1857-1857: WiringonMetadataFilterthrough Content is correct.Connects UI filter submissions to
filterMetadata. Good.
greg-in-a-box
force-pushed
the
mq-helper
branch
from
6b111bd to
f651e35
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/ContentExplorer.tsx (1)
397-417: Guard against undefined metadataTemplate in V1 path (callback invoked with 1 arg).V1 helpers likely call the success callback with only a Collection. In that case, metadataTemplate will be undefined at runtime and this code will overwrite state.metadataTemplate with undefined, causing regressions in the side panel/columns. Default the second parameter or coalesce with the existing state value before setState.
Apply this diff:
- showMetadataQueryResultsSuccessCallback = ( - metadataQueryCollection: Collection, - metadataTemplate: MetadataTemplate, - ): void => { + showMetadataQueryResultsSuccessCallback = ( + metadataQueryCollection: Collection, + metadataTemplate?: MetadataTemplate, + ): void => { const { nextMarker } = metadataQueryCollection; const { metadataQuery, features } = this.props; const { currentCollection, currentPageNumber, markers }: State = this.state; const cloneMarkers = [...markers]; if (nextMarker) { cloneMarkers[currentPageNumber + 1] = nextMarker; } + const effectiveTemplate = metadataTemplate ?? this.state.metadataTemplate; const nextState = { currentCollection: { ...currentCollection, ...metadataQueryCollection, percentLoaded: 100, }, markers: cloneMarkers, - metadataTemplate, + metadataTemplate: effectiveTemplate, };
♻️ Duplicate comments (8)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
48-66: Fix: robust type guards in convertFilterValuesToExternal to handle string/non-object values.Using
'enum' in valuewithout first verifyingtypeof value === 'object'can break when value is a primitive (e.g., string). Also, string filters currently fall through to a cast asRangeType | FloatType, which is incorrect.Apply this diff to harden the type checks and correctly pass through primitive values:
-export function convertFilterValuesToExternal(fields: FilterValues): ExternalFilterValues { - return Object.entries(fields).reduce<ExternalFilterValues>((acc, [key, field]) => { - const { value, options, fieldType } = field; - - // Transform the value based on its type - const transformedValue: ExternalMetadataFormFieldValue = - 'enum' in value && Array.isArray(value.enum) - ? value.enum // Convert enum type to string array - : (value as RangeType | FloatType); // Keep range/float objects as-is - - acc[key] = { - options, - fieldType, - value: transformedValue, - }; - - return acc; - }, {}); -} +export function convertFilterValuesToExternal(fields: FilterValues): ExternalFilterValues { + return Object.entries(fields).reduce<ExternalFilterValues>((acc, [key, field]) => { + const { value, options, fieldType } = field; + + const isObject = typeof value === 'object' && value !== null; + const isEnumValue = isObject && 'enum' in (value as unknown as Record<string, unknown>) && Array.isArray((value as any).enum); + const transformedValue: ExternalMetadataFormFieldValue = isEnumValue + ? (value as any).enum + : (value as any); // pass through strings, numbers, and range/float objects + + acc[key] = { + options, + fieldType, + value: transformedValue, + }; + return acc; + }, {}); +}src/elements/content-explorer/MetadataQueryBuilder.ts (1)
55-77: Fix range handling for zero bounds and non-numeric inputs.Truthiness checks skip valid 0 values;
Number(gt)may produceNaNand leak into params.Apply this diff:
- const { gt, lt } = filterValue.range; - const queryParams: { [key: string]: number } = {}; + const { gt, lt } = filterValue.range; + const queryParams: { [key: string]: number } = {}; const queries: string[] = []; - if (gt && lt) { + const hasGt = gt !== undefined && gt !== null && gt !== ''; + const hasLt = lt !== undefined && lt !== null && lt !== ''; + if (hasGt && hasLt) { // Both gt and lt: between values const argKeyGt = generateArgKey(fieldKey, (currentArgIndex += 1)); const argKeyLt = generateArgKey(fieldKey, (currentArgIndex += 1)); - queryParams[argKeyGt] = Number(gt); - queryParams[argKeyLt] = Number(lt); + const numGt = Number(gt); + const numLt = Number(lt); + if (!Number.isFinite(numGt) || !Number.isFinite(numLt)) { + return { queryParams: {}, queries: [], keysGenerated: 0 }; + } + queryParams[argKeyGt] = numGt; + queryParams[argKeyLt] = numLt; queries.push(`(${fieldKey} >= :${argKeyGt} AND ${fieldKey} <= :${argKeyLt})`); - } else if (gt) { + } else if (hasGt) { // Only gt: greater than const argKey = generateArgKey(fieldKey, (currentArgIndex += 1)); - queryParams[argKey] = Number(gt); + const num = Number(gt); + if (!Number.isFinite(num)) { + return { queryParams: {}, queries: [], keysGenerated: 0 }; + } + queryParams[argKey] = num; queries.push(`(${fieldKey} >= :${argKey})`); - } else if (lt) { + } else if (hasLt) { // Only lt: less than const argKey = generateArgKey(fieldKey, (currentArgIndex += 1)); - queryParams[argKey] = Number(lt); + const num = Number(lt); + if (!Number.isFinite(num)) { + return { queryParams: {}, queries: [], keysGenerated: 0 }; + } + queryParams[argKey] = num; queries.push(`(${fieldKey} <= :${argKey})`); }src/elements/content-explorer/MetadataQueryAPIHelper.ts (3)
307-316: Wrap merged query parts in parentheses to preserve boolean precedence.If either side contains OR clauses,
A OR B AND Cvs(A OR B) AND (C)changes semantics.Apply this diff:
- // Merge queries with AND operator - return `${customQuery} AND ${filterQuery}`; + // Merge queries with AND operator; wrap to preserve precedence + return `(${customQuery}) AND (${filterQuery})`;
268-275: Handle non-array string values properly.The code assumes
value[0]exists when value is truthy, butvaluecould be a string or an empty array. This could cause runtime errors or skip valid filters.Apply this diff to handle both string and array values:
case 'string': { - if (value && value[0]) { - const result = getStringFilter(value[0], key, argIndex); + // Handle string value which could be a string or string array + const stringValue = Array.isArray(value) ? value[0] : value; + if (stringValue) { + const result = getStringFilter(String(stringValue), key, argIndex); queryParams = mergeQueryParams(queryParams, result.queryParams); queries = mergeQueries(queries, result.queries); argIndex += result.keysGenerated; } break; }
332-334: Fix property name mismatch when destructuring query_params.The property is
query_paramsbut you're destructuringcustomQueryParamswhich is undefined after destructuring with the wrong key.Apply this diff to use the correct property name:
const { query: filterQuery, queryParams: filteredQueryParams } = this.buildMDQueryParams(fields); - const { query: customQuery, query_params: customQueryParams } = clonedQuery; + const { query: customQuery, query_params: customQueryParams = {} } = clonedQuery; const query = this.mergeQuery(customQuery, filterQuery);src/elements/content-explorer/ContentExplorer.tsx (3)
138-141: Omit exclusions are now correct.Nice fix—each prop is excluded via a proper union of string literal types. This resolves the earlier typing issue.
480-487: Fix TS type/sig mismatch: field typed as V1 is used to call V2 signature.this.metadataQueryAPIHelper is declared as V1 but is used to call a V2-only 4-arg fetchMetadataQueryResults, which can yield “Expected 3 arguments, but got 4”. Call on a local V2-typed instance, then assign it to the field.
Apply this diff:
- this.metadataQueryAPIHelper = new MetadataQueryAPIHelperV2(this.api); - this.metadataQueryAPIHelper.fetchMetadataQueryResults( - metadataQueryClone, - metadataFilters, - this.showMetadataQueryResultsSuccessCallback, - this.errorCallback, - ); + const helperV2 = new MetadataQueryAPIHelperV2(this.api); + this.metadataQueryAPIHelper = helperV2; + helperV2.fetchMetadataQueryResults( + metadataQueryClone, + metadataFilters, + this.showMetadataQueryResultsSuccessCallback, + this.errorCallback, + );Run this to confirm both helper signatures and catch any remaining callsites:
#!/bin/bash set -euo pipefail # Show signatures of both helpers rg -nP --glob '!**/dist/**' 'class\s+MetadataQueryAPIHelperV2|class\s+MetadataQueryAPIHelper' -C1 rg -nP --glob '!**/dist/**' '\bfetchMetadataQueryResults\s*\(' -C2 # Call sites rg -nP --glob '!**/dist/**' '\.fetchMetadataQueryResults\s*\(' -C2
1702-1704: Reset marker pagination when filters change to avoid stale/incorrect pages.Applying new metadataFilters should clear markers and reset currentPageNumber. Otherwise, you can reuse a marker from a prior query and show the wrong page.
Apply this diff:
- filterMetadata = (fields: ExternalFilterValues) => { - this.setState({ metadataFilters: fields }, this.refreshCollection); - }; + filterMetadata = (fields: ExternalFilterValues) => { + this.setState( + { + metadataFilters: fields, + currentPageNumber: 0, + markers: [], + }, + this.refreshCollection, + ); + };
🧹 Nitpick comments (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
907-915: Optional: also reset markers when sorting within VIEW_METADATA.A sort change alters the query; keeping old markers risks mixing pagination from the previous sort order.
Apply this diff:
sort = (sortBy: SortBy, sortDirection: SortDirection) => { const { currentCollection: { id }, view, }: State = this.state; if (id || view === VIEW_METADATA) { - this.setState({ sortBy, sortDirection }, this.refreshCollection); + this.setState( + { + sortBy, + sortDirection, + ...(view === VIEW_METADATA && { currentPageNumber: 0, markers: [] }), + }, + this.refreshCollection, + ); } };
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
package.json(1 hunks)src/elements/common/__mocks__/mockMetadata.ts(7 hunks)src/elements/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(10 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(4 hunks)src/elements/content-explorer/MetadataQueryBuilder.ts(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(4 hunks)src/elements/content-explorer/__tests__/Content.test.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(9 hunks)src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(6 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/elements/common/mocks/mockMetadata.ts
- src/elements/content-explorer/Content.tsx
- src/elements/content-explorer/tests/Content.test.tsx
- src/elements/content-explorer/tests/MetadataQueryBuilder.test.ts
- package.json
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js (7)
metadataQueryAPIHelper(15-15)metadataInstanceId1(19-19)metadataQueryResponse(70-116)nextMarker(69-69)getSchemaByTemplateKeyFunc(191-191)queryMetadataFunc(192-192)mdQuery(207-218)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (3)
MetadataViewContainerProps(73-78)ExternalFilterValues(17-24)convertFilterValuesToExternal(48-66)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
ExternalFilterValues(17-24)src/elements/content-explorer/MetadataQueryBuilder.ts (7)
generateArgKey(30-33)getRangeFilter(48-90)mergeQueryParams(19-24)mergeQueries(26-28)getMimeTypeFilter(123-151)getSelectFilter(92-121)getStringFilter(37-46)
⏰ 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 (8)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
385-509: LGTM! Comprehensive test coverage for convertFilterValuesToExternal.The test suite thoroughly covers all edge cases including enum conversions, range values preservation, mixed field types, empty filter objects, and multiSelect handling. This gives confidence in the converter's robustness.
src/elements/content-explorer/MetadataQueryBuilder.ts (1)
138-138: Suffix removal logic validated
The implementationv.endsWith('Type') ? v.slice(0, -4) : vensures that only strings ending with the literal suffix"Type"have those characters removed. The accompanying unit tests inMetadataQueryBuilder.test.tsexplicitly cover cases both with (['pdfType', 'docType', 'txtType']) and without (['pdf', 'doc']) the"Type"suffix, confirming the intended behavior. No other string patterns or unintended slice operations were found in the codebase.src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
558-770: LGTM! Excellent test coverage for buildMDQueryParams.The test suite comprehensively covers all filter types (date/float ranges, enum, multiSelect, string, mimetype), edge cases (null/undefined values, empty arrays, unknown field types), and complex scenarios (multiple filters with AND logic). This provides strong validation of the filter building logic.
src/elements/content-explorer/ContentExplorer.tsx (5)
96-96: LGTM: New types are correctly imported and used.ExternalFilterValues and MetadataViewContainerProps wiring looks consistent with the later state/prop usage.
184-184: State plumbing for metadataFilters looks good.Initializing to an empty object is fine and keeps the V2 path simple.
Also applies to: 310-311
446-447: LGTM: metadataFilters are included in the query context.Destructuring and forwarding the filters is correct and keeps V1 behavior unchanged.
495-499: V1 call path is preserved.With the success callback now guarding the template param, this path remains safe and types should be satisfied.
1857-1857: LGTM: onMetadataFilter is correctly wired to Content.This plumbs user interactions through to the stateful handler.
greg-in-a-box
force-pushed
the
mq-helper
branch
2 times, most recently
from
ab1cb9f to
5c4c0f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
48-66: Harden enum detection: guard against non-object values and pass through primitives.Using
'enum' in valuewithout verifyingvalueis a non-null object can throw whenvalueis a primitive (string/number). It also incorrectly forces a cast to RangeType|FloatType. Fix by checking object-ness first and returning primitives as-is. This also correctly supports string single-values flowing to downstream builders.export function convertFilterValuesToExternal(fields: FilterValues): ExternalFilterValues { return Object.entries(fields).reduce<ExternalFilterValues>((acc, [key, field]) => { const { value, options, fieldType } = field; - // Transform the value based on its type - const transformedValue: ExternalMetadataFormFieldValue = - 'enum' in value && Array.isArray(value.enum) - ? value.enum // Convert enum type to string array - : (value as RangeType | FloatType); // Keep range/float objects as-is + // Transform the value based on its type + const isObject = typeof value === 'object' && value !== null; + const maybeRecord = value as Record<string, unknown>; + const isEnumArray = isObject && 'enum' in maybeRecord && Array.isArray((maybeRecord as any).enum); + const transformedValue: ExternalMetadataFormFieldValue = isEnumArray + ? ((maybeRecord as any).enum as string[]) + : (value as any); // pass through strings, numbers, and range/float objects acc[key] = { options, fieldType, value: transformedValue, }; return acc; }, {}); }src/elements/content-explorer/ContentExplorer.tsx (3)
138-141: Omit exclusions fixed.Correct union of string literal keys; this resolves the prior typing issue.
480-487: Call V2 helper via a V2-typed reference to satisfy TS.
this.metadataQueryAPIHelperis a union (V1|V2). Calling a 4-arg signature on the union can fail TS narrowing at the call site. Instantiate a V2-typed local, call it, then assign to the union field.- this.metadataQueryAPIHelper = new MetadataQueryAPIHelperV2(this.api); - this.metadataQueryAPIHelper.fetchMetadataQueryResults( - metadataQueryClone, - this.showMetadataQueryResultsSuccessCallback, - this.errorCallback, - metadataFilters, - ); + const helperV2 = new MetadataQueryAPIHelperV2(this.api); + this.metadataQueryAPIHelper = helperV2; + helperV2.fetchMetadataQueryResults( + metadataQueryClone, + this.showMetadataQueryResultsSuccessCallback, + this.errorCallback, + metadataFilters, + );
1702-1705: Reset marker-based pagination when filters change.Applying new filters should start a fresh pagination sequence; otherwise stale markers may fetch the wrong page for a different query.
- filterMetadata = (fields: ExternalFilterValues) => { - this.setState({ metadataFilters: fields }, this.refreshCollection); - }; + filterMetadata = (fields: ExternalFilterValues) => { + this.setState( + { + metadataFilters: fields, + currentPageNumber: 0, + markers: [], + }, + this.refreshCollection, + ); + };Optional follow-up: apply the same reset when sort changes in
VIEW_METADATAto guarantee correct pagination for a new sort order. I can patch that if you want.
🧹 Nitpick comments (1)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
385-508: Add a guard-rail test for single primitive values.Given the converter now passes through primitives, add a test where an enum filter is a single string (not array) to avoid regressions and ensure downstream builders keep supporting it.
@@ describe('convertFilterValuesToExternal', () => { test('should handle multiSelect values', () => { const internalFilters = { 'category-filter': { fieldType: 'multiSelect' as const, options: [ { key: 'tech', id: 'tech1' }, { key: 'finance', id: 'finance1' }, ], value: { enum: ['tech', 'finance'] }, }, }; const result = convertFilterValuesToExternal(internalFilters); expect(result['category-filter'].value).toEqual(['tech', 'finance']); expect(result['category-filter'].fieldType').toBe('multiSelect'); }); + + test('should pass through single string enum value without error', () => { + const internalFilters = { + status: { + fieldType: 'enum' as const, + value: 'active', + }, + } as any; + const result = convertFilterValuesToExternal(internalFilters); + expect(result.status.value).toBe('active'); + expect(result.status.fieldType).toBe('enum'); + });
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
package.json(1 hunks)src/elements/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(11 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(4 hunks)src/elements/content-explorer/MetadataQueryBuilder.ts(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(4 hunks)src/elements/content-explorer/__tests__/Content.test.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(8 hunks)src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(6 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/elements/content-explorer/tests/Content.test.tsx
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
- src/elements/content-explorer/tests/MetadataQueryBuilder.test.ts
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
- src/elements/content-explorer/MetadataQueryBuilder.ts
- src/elements/content-explorer/Content.tsx
- package.json
- src/elements/content-explorer/MetadataQueryAPIHelper.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (3)
MetadataViewContainerProps(73-78)ExternalFilterValues(17-24)convertFilterValuesToExternal(48-66)
src/elements/content-explorer/ContentExplorer.tsx (2)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
MetadataViewContainerProps(73-78)ExternalFilterValues(17-24)src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
MetadataQueryAPIHelper(48-338)
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js (6)
metadataInstanceId1(19-19)metadataQueryResponse(70-116)nextMarker(69-69)getSchemaByTemplateKeyFunc(191-191)queryMetadataFunc(192-192)mdQuery(207-218)
⏰ 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 (6)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
68-71: Wrapper indirection is clean.Keeping the conversion behind
transformInternalFieldsToPublicis a good seam for future behavior changes.
115-124: Submit flow order looks right.Invoking
onMetadataFilterfirst and then conditionally callingonFilterSubmitpreserves the new public API while maintaining existing behavior.src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (2)
129-147: Good assertion of multi-select chip behavior.Verifies progressive accumulation and correct external shape (string[]). Solid coverage.
149-187: End-to-end callback coverage is on point.Asserting both
onMetadataFilterandonFilterSubmitget called with identical payloads validates the new plumbing.src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (2)
358-367: Error path verified.Nice negative-path test ensuring
errorCallbackis invoked when the query promise rejects.
558-770: Thorough query-builder coverage.Great breadth across empty/null, ranges, select types, string ILIKE, mimetype special-case, AND merges, and unknown types. This will catch most regressions in the MD query construction.
greg-in-a-box
force-pushed
the
mq-helper
branch
from
5c4c0f9 to
f0bed74
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
292-301: Preserve boolean precedence by wrapping query parts in parentheses.When merging queries with AND, wrapping each part in parentheses ensures correct precedence if either query contains OR operators.
mergeQuery = (customQuery: string, filterQuery: string): string => { if (!customQuery) { return filterQuery; } if (!filterQuery) { return customQuery; } - // Merge queries with AND operator - return `${customQuery} AND ${filterQuery}`; + // Merge queries with AND operator; wrap to preserve precedence + return `(${customQuery}) AND (${filterQuery})`; };
316-323: Fix incorrect property access for query_params.The code destructures
customQueryParamsfromquery_paramsbut the call tomergeQueryParamsexpects an object. Ensure proper default value handling.if (fields) { const { query: filterQuery, queryParams: filteredQueryParams } = this.buildMDQueryParams(fields); - const { query: customQuery, query_params: customQueryParams } = clonedQuery; + const { query: customQuery, query_params: customQueryParams = {} } = clonedQuery; const query = this.mergeQuery(customQuery, filterQuery); const queryParams = mergeQueryParams(filteredQueryParams, customQueryParams); if (query) { clonedQuery.query = query; clonedQuery.query_params = queryParams; } }src/elements/content-explorer/ContentExplorer.tsx (2)
483-488: Add missing fields argument to non-V2 fetchMetadataQueryResults call.The V2 branch correctly passes
metadataFiltersas the fourth parameter, but the non-V2 branch still uses the old three-parameter signature. This inconsistency could cause runtime errors.} else { metadataQueryClone.order_by = [ { field_key: sortBy, direction: sortDirection, }, ]; this.metadataQueryAPIHelper = new MetadataQueryAPIHelper(this.api); this.metadataQueryAPIHelper.fetchMetadataQueryResults( metadataQueryClone, + metadataFilters, this.showMetadataQueryResultsSuccessCallback, this.errorCallback, ); }Also applies to: 497-501
1704-1706: Reset pagination state when filters change to avoid stale results.When new filters are applied, the current page number and markers should be reset to avoid using stale pagination markers from a different query.
filterMetadata = (fields: ExternalFilterValues) => { - this.setState({ metadataFilters: fields }, this.refreshCollection); + this.setState( + { + metadataFilters: fields, + currentPageNumber: 0, + markers: [], + }, + this.refreshCollection, + ); };src/elements/content-explorer/MetadataQueryBuilder.ts (1)
55-77: Range handling skips valid zero bounds and can inject NaN into params.Truthiness checks treat 0 as false and
Number(x)can yieldNaN, which then leaks into queryParams.Apply this diff to handle 0 correctly, validate numeric inputs, and keep inclusive semantics:
- if (filterValue && typeof filterValue === 'object' && 'range' in filterValue && filterValue.range) { - const { gt, lt } = filterValue.range; - const queryParams: { [key: string]: number } = {}; - const queries: string[] = []; - - if (gt && lt) { + if (filterValue && typeof filterValue === 'object' && 'range' in filterValue && filterValue.range) { + const { gt, lt } = filterValue.range; + const queryParams: { [key: string]: number } = {}; + const queries: string[] = []; + + const hasGt = gt !== undefined && gt !== null && gt !== ''; + const hasLt = lt !== undefined && lt !== null && lt !== ''; + if (hasGt && hasLt) { // Both gt and lt: between values const argKeyGt = generateArgKey(fieldKey, (currentArgIndex += 1)); const argKeyLt = generateArgKey(fieldKey, (currentArgIndex += 1)); - queryParams[argKeyGt] = Number(gt); - queryParams[argKeyLt] = Number(lt); + const numGt = Number(gt); + const numLt = Number(lt); + if (!Number.isFinite(numGt) || !Number.isFinite(numLt)) { + return { queryParams: {}, queries: [], keysGenerated: 0 }; + } + queryParams[argKeyGt] = numGt; + queryParams[argKeyLt] = numLt; queries.push(`(${fieldKey} >= :${argKeyGt} AND ${fieldKey} <= :${argKeyLt})`); - } else if (gt) { + } else if (hasGt) { // Only gt: greater than const argKey = generateArgKey(fieldKey, (currentArgIndex += 1)); - queryParams[argKey] = Number(gt); + const num = Number(gt); + if (!Number.isFinite(num)) { + return { queryParams: {}, queries: [], keysGenerated: 0 }; + } + queryParams[argKey] = num; queries.push(`(${fieldKey} >= :${argKey})`); - } else if (lt) { + } else if (hasLt) { // Only lt: less than const argKey = generateArgKey(fieldKey, (currentArgIndex += 1)); - queryParams[argKey] = Number(lt); + const num = Number(lt); + if (!Number.isFinite(num)) { + return { queryParams: {}, queries: [], keysGenerated: 0 }; + } + queryParams[argKey] = num; queries.push(`(${fieldKey} <= :${argKey})`); }
🧹 Nitpick comments (4)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
220-290: Simplify buildMDQueryParams by extracting type handlers.The method has grown complex with nested switch statements. Consider extracting each type handler into separate methods for better maintainability.
buildMDQueryParams = (filters: ExternalFilterValues) => { let argIndex = 0; let queries: string[] = []; let queryParams: { [key: string]: number | Date | string } = {}; if (filters) { Object.keys(filters).forEach(key => { const filter = filters[key]; if (!filter) { return; } const { fieldType, value } = filter; + if (value == null) { + return; + } switch (fieldType) { case 'date': case 'float': { - if (typeof value === 'object' && value !== null && 'range' in value) { - const result = getRangeFilter(value, key, argIndex); - queryParams = mergeQueryParams(queryParams, result.queryParams); - queries = mergeQueries(queries, result.queries); - argIndex += result.keysGenerated; - break; - } - break; + const result = this.handleDateFloatFilter(value, key, argIndex); + queryParams = mergeQueryParams(queryParams, result.queryParams); + queries = mergeQueries(queries, result.queries); + argIndex += result.keysGenerated; + break; } case 'enum': case 'multiSelect': { - const arrayValue = Array.isArray(value) ? value.map(v => String(v)) : [String(value)]; - let result; - if (key === 'mimetype-filter') { - result = getMimeTypeFilter(arrayValue, key, argIndex); - } else { - result = getSelectFilter(arrayValue, key, argIndex); - } - queryParams = mergeQueryParams(queryParams, result.queryParams); - queries = mergeQueries(queries, result.queries); - argIndex += result.keysGenerated; - break; + const result = this.handleSelectFilter(value, key, argIndex, fieldType); + queryParams = mergeQueryParams(queryParams, result.queryParams); + queries = mergeQueries(queries, result.queries); + argIndex += result.keysGenerated; + break; } case 'string': { - if (value && value[0]) { - const result = getStringFilter(value[0], key, argIndex); - queryParams = mergeQueryParams(queryParams, result.queryParams); - queries = mergeQueries(queries, result.queries); - argIndex += result.keysGenerated; - } - break; + const result = this.handleStringFilter(value, key, argIndex); + queryParams = mergeQueryParams(queryParams, result.queryParams); + queries = mergeQueries(queries, result.queries); + argIndex += result.keysGenerated; + break; } default: break; } }); } const query = queries.reduce((acc, curr, index) => { if (index > 0) { acc += ` AND ${curr}`; } else { acc = curr; } return acc; }, ''); return { queryParams, query, }; }; +private handleDateFloatFilter = (value: any, key: string, argIndex: number) => { + if (typeof value === 'object' && value !== null && 'range' in value) { + return getRangeFilter(value, key, argIndex); + } + return { queryParams: {}, queries: [], keysGenerated: 0 }; +}; + +private handleSelectFilter = (value: any, key: string, argIndex: number, fieldType: string) => { + const arrayValue = Array.isArray(value) ? value.map(v => String(v)) : [String(value)]; + if (key === 'mimetype-filter') { + return getMimeTypeFilter(arrayValue, key, argIndex); + } + return getSelectFilter(arrayValue, key, argIndex); +}; + +private handleStringFilter = (value: any, key: string, argIndex: number) => { + const stringValue = Array.isArray(value) ? value[0] : value; + if (stringValue) { + return getStringFilter(stringValue, key, argIndex); + } + return { queryParams: {}, queries: [], keysGenerated: 0 }; +};src/elements/content-explorer/MetadataQueryBuilder.ts (3)
37-46: Avoid generating a no-op “ILIKE '%%'” filter for empty input.Empty or whitespace-only strings currently produce an always-true predicate, which can degrade performance and change semantics.
Apply this diff to short-circuit on empty input and reuse the trimmed value:
export const getStringFilter = (filterValue: string, fieldKey: string, argIndexStart: number): QueryResult => { let currentArgIndex = argIndexStart; - const argKey = generateArgKey(fieldKey, (currentArgIndex += 1)); - return { - queryParams: { [argKey]: `%${escapeValue(filterValue)}%` }, - queries: [`(${fieldKey} ILIKE :${argKey})`], - keysGenerated: currentArgIndex - argIndexStart, - }; + const trimmed = filterValue?.trim?.() ?? ''; + if (trimmed.length === 0) { + return { queryParams: {}, queries: [], keysGenerated: 0 }; + } + const argKey = generateArgKey(fieldKey, (currentArgIndex += 1)); + return { + queryParams: { [argKey]: `%${escapeValue(trimmed)}%` }, + queries: [`(${fieldKey} ILIKE :${argKey})`], + keysGenerated: currentArgIndex - argIndexStart, + }; };
30-33: Validate field identifiers to prevent malformed queries.
fieldKeyis interpolated directly into the query. Even if user-controlled inputs are unlikely, defensive validation avoids malformed or surprising queries (e.g., hyphens, spaces).If acceptable, introduce a small guard and apply it in all builders:
// near generateArgKey const ensureSafeFieldKey = (key: string): string => { // allow words and dot for nested paths like item.extension if (!/^[\w.]+$/.test(key)) { throw new Error(`Invalid field key: ${key}`); } return key; };Then use
const safeField = ensureSafeFieldKey(fieldKey);in each builder and referencesafeFieldin query strings.Would this be compatible with all fields used by MetadataView (including metadata template paths)?
Also applies to: 37-46, 55-77, 92-121, 123-151
123-151: Tests: add coverage for new/edge cases.
getRangeFilter: gt=0, lt=0, gt='0', lt='0', non-numeric inputs (‘abc’), NaN propagation (should short-circuit).getStringFilter: empty string and whitespace-only (no filter emitted).getMimeTypeFilter: non-string entries (numbers), values with and withoutTypesuffix, and optional case normalization.- Ensure argIndex continuity across mixed builder invocations (keysGenerated math).
I can draft test cases mirroring the above and wire them into your existing
MetadataQueryBuilder.test.ts. Want me to push a patch?
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
package.json(1 hunks)src/elements/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(11 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(4 hunks)src/elements/content-explorer/MetadataQueryBuilder.ts(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(4 hunks)src/elements/content-explorer/__tests__/Content.test.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(8 hunks)src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(6 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 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/Content.tsx
- src/elements/content-explorer/tests/MetadataQueryBuilder.test.ts
- package.json
- src/elements/content-explorer/MetadataViewContainer.tsx
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-15T14:42:01.840Z
Learnt from: jpan-box
PR: box/box-ui-elements#4227
File: src/elements/common/sub-header/SubHeader.tsx:19-19
Timestamp: 2025-08-15T14:42:01.840Z
Learning: In SubHeader.tsx, the bulkItemActionMenuProps prop is intentionally required (not optional) because there will always be a default "Download" action available, ensuring the prop is never undefined in actual usage.
Applied to files:
src/elements/content-explorer/ContentExplorer.tsx
🧬 Code graph analysis (4)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (3)
MetadataViewContainerProps(73-78)ExternalFilterValues(17-24)convertFilterValuesToExternal(48-66)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
ExternalFilterValues(17-24)src/elements/content-explorer/MetadataQueryBuilder.ts (6)
getRangeFilter(48-90)mergeQueryParams(19-24)mergeQueries(26-28)getMimeTypeFilter(123-151)getSelectFilter(92-121)getStringFilter(37-46)
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js (6)
metadataInstanceId1(19-19)metadataQueryResponse(70-116)nextMarker(69-69)getSchemaByTemplateKeyFunc(191-191)queryMetadataFunc(192-192)mdQuery(207-218)
src/elements/content-explorer/ContentExplorer.tsx (2)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
MetadataViewContainerProps(73-78)ExternalFilterValues(17-24)src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
MetadataQueryAPIHelper(48-338)
⏰ 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 (18)
src/elements/content-explorer/__tests__/Content.test.tsx (1)
33-33: LGTM! Prop addition aligns with new filtering capability.The addition of
onMetadataFilter: jest.fn()tomockPropscorrectly mocks the new filtering callback.src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (5)
6-10: LGTM! Export additions enable proper testing of the filtering API.The import changes correctly expose
convertFilterValuesToExternalandExternalFilterValuesfor comprehensive testing.
21-21: LGTM! Fixed typo in test data.The correction from
' name'to'name'fixes the stray space in the key.
96-98: LGTM! Proper test cleanup between test runs.The addition of
beforeEachwithjest.clearAllMocks()ensures test isolation.
145-146: LGTM! Updated assertion style for clarity.Using dot notation (
firstCall.role.value) improves readability over bracket notation.
385-508: LGTM! Comprehensive test coverage for filter conversion logic.The new test suite for
convertFilterValuesToExternalthoroughly covers:
- Enum to string array conversion
- Range value preservation
- Mixed field types
- Edge cases (empty filters, empty arrays)
- Options field preservation
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (4)
13-13: LGTM! Test path correctly reflects new location.The describe string update from
features/metadata-based-viewtoelements/content-explorerreflects the module's new location.
229-250: LGTM! Comprehensive edge case coverage for flattenMetadata.Good test coverage for:
- Empty template fields array
- Missing metadata fields in the response
- Proper handling of undefined values
558-770: LGTM! Excellent test coverage for buildMDQueryParams.Comprehensive test suite covering:
- Empty/null filters
- Date/float range filters with gt/lt operators
- Enum and multiSelect filters
- String search filters
- Special mimetype filter handling
- Multiple filter combinations
- Edge cases (null values, empty strings, empty arrays)
772-865: LGTM! Good coverage for verifyQueryFields with external filters.The tests properly validate:
- Query building with filters
- Query parameter merging
- AND operator logic for multiple filters
- Merging existing query_params with filter params
- Required field inclusion (FIELD_NAME, FIELD_EXTENSION)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
30-38: LGTM! Clean integration of external filtering utilities.The imports are properly organized and the new dependencies from
MetadataQueryBuilderandMetadataViewContainerare correctly integrated.
189-204: LGTM! Method signature properly extended for filtering support.The addition of the optional
fieldsparameter maintains backward compatibility while enabling the new filtering functionality.src/elements/content-explorer/ContentExplorer.tsx (5)
97-97: LGTM! Clean import of filtering types.The import properly brings in both
ExternalFilterValuesandMetadataViewContainerPropsfrom the correct location.
140-143: LGTM! Omit type correctly excludes internal props.The
Omitutility properly excludes the internal props that are managed by the ContentExplorer component.
186-186: LGTM! State properly initialized for metadata filtering.The new state fields are correctly typed and initialized with appropriate default values.
Also applies to: 217-217, 312-312
399-402: LGTM! Callback signature properly updated for metadata template.The success callback now correctly receives and stores the metadata template in state.
1861-1861: LGTM! Filter callback properly wired to Content component.The
onMetadataFilterprop is correctly passed to the Content component and connected to thefilterMetadatamethod.src/elements/content-explorer/MetadataQueryBuilder.ts (1)
19-28: LGTM: merge helpers are simple and correct.Shallow merge semantics for params and concatenation for queries are appropriate here. No issues.
jfox-box
left a comment
jfox-box
left a comment
There was a problem hiding this comment.
Overall LGTM, some minor comments. Is it worth testing that the argument indexing is properly producing unique field keys, in case it gets broken by a future change?
greg-in-a-box
force-pushed
the
mq-helper
branch
from
f0bed74 to
289e2f2
Compare
greg-in-a-box
force-pushed
the
mq-helper
branch
2 times, most recently
from
79e281d to
3b80d43
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/MetadataViewContainer.tsx (1)
67-79: Map FIELD_ITEM_NAME ⇄ ITEM_FILTER_NAME on initial values, sucka.Reverse mapping is missing; external keys like
item.namewon’t hydrate internalitem_name.Apply:
function transformInitialFilterValuesToInternal( publicValues?: ExternalFilterValues, ): Record<string, { value: MetadataFormFieldValue }> | undefined { if (!publicValues) return undefined; - return Object.entries(publicValues).reduce<Record<string, { value: MetadataFormFieldValue }>>( - (acc, [key, { value }]) => { - acc[key] = Array.isArray(value) ? { value: { enum: value } } : { value }; + return Object.entries(publicValues).reduce<Record<string, { value: MetadataFormFieldValue }>>( + (acc, [key, { value }]) => { + const internalKey = key === FIELD_ITEM_NAME ? ITEM_FILTER_NAME : key; + acc[internalKey] = Array.isArray(value) ? { value: { enum: value } } : { value: value as MetadataFormFieldValue }; return acc; }, {}, ); }
♻️ Duplicate comments (5)
src/elements/content-explorer/MetadataViewContainer.tsx (4)
236-246: Recognize legacy item name keys in filters, fool.Avoid duplicate chips when template already exposes
item.nameorname.Apply:
- const hasItemNameField = filteredFields.some((field: MetadataTemplateField) => field.key === ITEM_FILTER_NAME); + const hasItemNameField = filteredFields.some((field: MetadataTemplateField) => + [ITEM_FILTER_NAME, FIELD_ITEM_NAME, 'name'].includes(field.key), + );
81-99: Harden convertFilterValuesToExternal; don’t blow up on primitives, fool.Using
'in'on non-objects throws. Pass through strings/numbers; only unwrap enum arrays when value is an object.Apply:
export function convertFilterValuesToExternal(fields: FilterValues): ExternalFilterValues { return Object.entries(fields).reduce<ExternalFilterValues>((acc, [key, field]) => { const { value, options, fieldType } = field; - // Transform the value based on its type - const transformedValue: ExternalMetadataFormFieldValue = - 'enum' in value && Array.isArray(value.enum) - ? value.enum // Convert enum type to string array - : (value as RangeType | FloatType); // Keep range/float objects as-is + // Transform the value based on its type (be safe with primitives) + const isObject = typeof value === 'object' && value !== null; + const isEnumArray = isObject && 'enum' in (value as any) && Array.isArray((value as any).enum); + const transformedValue: ExternalMetadataFormFieldValue = isEnumArray + ? (value as any).enum + : (value as any); // pass through primitives and range/float objects - acc[key === ITEM_FILTER_NAME ? FIELD_ITEM_NAME : key] = { + acc[key === ITEM_FILTER_NAME ? FIELD_ITEM_NAME : key] = { options, fieldType, value: transformedValue, }; return acc; }, {}); }
131-146: Guard columns; keep Name first without dupes.
cloneDeep(columns)can be undefined; also promote existing Name column to front instead of inserting duplicates.Apply:
- const clonedColumns = cloneDeep(columns); + const clonedColumns: Column[] = cloneDeep(columns || []); // Filter columns based on fieldsToShow if provided let filteredColumns = clonedColumns; @@ - const hasItemNameField = filteredColumns.some((col: Column) => col.id === FIELD_ITEM_NAME); - - if (!hasItemNameField) { - filteredColumns.unshift({ + const nameIdx = filteredColumns.findIndex((col: Column) => col?.id === FIELD_ITEM_NAME); + if (nameIdx === -1) { + filteredColumns.unshift({ allowsSorting: true, id: FIELD_ITEM_NAME, isItemMetadata: true, isRowHeader: true, minWidth: 250, maxWidth: 250, textValue: formatMessage(messages.metadataItemName), type: 'string', } as Column); + } else if (nameIdx > 0) { + filteredColumns.unshift(...filteredColumns.splice(nameIdx, 1)); }Also applies to: 148-195
106-114: Make onMetadataFilter optional to avoid breaking consumers.Default to noop and guard the call.
Apply:
export interface MetadataViewContainerProps extends Omit<MetadataViewProps, 'items' | 'actionBarProps'> { @@ - onMetadataFilter: (fields: ExternalFilterValues) => void; + onMetadataFilter?: (fields: ExternalFilterValues) => void; @@ - onMetadataFilter(transformed); + onMetadataFilter?.(transformed);Also applies to: 270-279
src/elements/content-explorer/ContentExplorer.tsx (1)
1740-1743: Reset marker pagination on filter submit, or face stale pages, fool.New filters must start from page 0 with fresh markers.
Apply:
- filterMetadata = (fields: ExternalFilterValues) => { - this.setState({ metadataFilters: fields }, this.refreshCollection); - }; + filterMetadata = (fields: ExternalFilterValues) => { + this.setState( + { + metadataFilters: fields, + currentPageNumber: 0, + markers: [], + }, + this.refreshCollection, + ); + };
🧹 Nitpick comments (7)
src/elements/content-explorer/ContentExplorer.tsx (1)
945-953: Optional: also reset markers on sort changes in metadata view.New sort order shouldn’t reuse old markers.
Apply:
- sort = (sortBy: SortBy | Key, sortDirection: SortDirection) => { + sort = (sortBy: SortBy | Key, sortDirection: SortDirection) => { const { - currentCollection: { id }, - view, + currentCollection: { id }, + view, }: State = this.state; - if (id || view === VIEW_METADATA) { - this.setState({ sortBy, sortDirection }, this.refreshCollection); + if (id || view === VIEW_METADATA) { + const reset = view === VIEW_METADATA ? { currentPageNumber: 0, markers: [] } : {}; + this.setState({ sortBy, sortDirection, ...reset }, this.refreshCollection); } };src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (6)
314-321: Error propagation test reads well—consider clearing prior mock state.Since queryMetadataFunc is shared across tests, add mockClear to avoid bleed-over.
test('should handle API errors properly', async () => { const error = new Error('API Error'); + queryMetadataFunc.mockClear(); queryMetadataFunc.mockImplementationOnce((query, resolve, reject) => { reject(error); });
545-559: Non-array fields normalization—add permission assertion too, fool.Minor improvement: also assert FIELD_PERMISSIONS exists for parity.
expect(isArray(updatedMetadataQuery.fields)).toBe(true); expect(includes(updatedMetadataQuery.fields, FIELD_ITEM_NAME)).toBe(true); expect(includes(updatedMetadataQuery.fields, FIELD_EXTENSION)).toBe(true); +expect(includes(updatedMetadataQuery.fields, FIELD_PERMISSIONS)).toBe(true);
578-589: Clarify GT/LT semantics or expectations.Tests expect >= for gt and <= for lt. If that’s intended (inclusive), rename test titles for clarity; if not, adjust code to strict operators. Don’t confuse the fools reading tests later.
-test('should handle date/float field with greater than filter', () => { +test('should handle date/float field with greater-than-or-equal filter', () => { ... -test('should handle date/float field with less than filter', () => { +test('should handle date/float field with less-than-or-equal filter', () => {Also applies to: 591-603, 604-616
659-670: Avoid overloading “name”—use a neutral metadata key.“name” can be confused with item.name reserved field. Use “title” (or similar) to keep intent clean, sucker.
- name: { + title: { fieldType: 'string', - value: ['search term'], + value: ['search term'], }, ... - expect(result.query).toBe('(name ILIKE :arg_name_1)'); - expect(result.queryParams.arg_name_1).toBe('%search term%'); + expect(result.query).toBe('(title ILIKE :arg_title_1)'); + expect(result.queryParams.arg_title_1).toBe('%search term%');
672-684: Mime-type filter special-casing—consider normalization.If inputs vary in case (“PDF”, “Pdf”), normalize to lowercase in params; add a test to lock it down.
test('should handle mimetype filter specifically', () => { const filters = { 'mimetype-filter': { fieldType: 'enum', - value: ['pdf', 'doc'], + value: ['PDF', 'doc'], }, }; const result = metadataQueryAPIHelper.buildMDQueryParams(filters); expect(result.query).toBe('(item.extension IN (:arg_mimetype_filter_1, :arg_mimetype_filter_2))'); - expect(result.queryParams.arg_mimetype_filter_1).toBe('pdf'); + expect(result.queryParams.arg_mimetype_filter_1).toBe('pdf'); expect(result.queryParams.arg_mimetype_filter_2).toBe('doc'); });
735-747: Unknown field type assertions are brittle—assert emptiness instead.Checking arg_field_1/2 by name ties you to internal numbering.
- expect(result.query).toBeFalsy(); - expect(result.queryParams.arg_field_1).toBeUndefined(); - expect(result.queryParams.arg_field_2).toBeUndefined(); + expect(result.query).toBe(''); + expect(result.queryParams).toEqual({});
📜 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 (2)
i18n/en-US.propertiesis excluded by!i18n/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
package.json(2 hunks)src/elements/common/__mocks__/mockMetadata.ts(1 hunks)src/elements/common/messages.js(1 hunks)src/elements/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(13 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(7 hunks)src/elements/content-explorer/MetadataQueryBuilder.ts(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(7 hunks)src/elements/content-explorer/__tests__/Content.test.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(11 hunks)src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(6 hunks)src/elements/content-explorer/stories/MetadataView.stories.tsx(2 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- src/elements/content-explorer/tests/Content.test.tsx
- package.json
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
- src/elements/common/messages.js
- src/elements/content-explorer/stories/MetadataView.stories.tsx
- src/elements/content-explorer/tests/MetadataQueryBuilder.test.ts
- src/elements/content-explorer/Content.tsx
- src/elements/content-explorer/MetadataQueryBuilder.ts
- src/elements/content-explorer/MetadataQueryAPIHelper.ts
- src/elements/content-explorer/tests/MetadataViewContainer.test.tsx
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
- src/elements/common/mocks/mockMetadata.ts
🧰 Additional context used
🧠 Learnings (3)
📚 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/MetadataViewContainer.tsxsrc/elements/content-explorer/ContentExplorer.tsx
📚 Learning: 2025-08-28T15:38:35.075Z
Learnt from: greg-in-a-box
PR: box/box-ui-elements#4235
File: src/elements/content-explorer/MetadataViewContainer.tsx:106-123
Timestamp: 2025-08-28T15:38:35.075Z
Learning: In the box/metadata-view Column interface used in MetadataViewContainer.tsx, the correct property name for enabling sorting is `allowsSorting`, not `allowSorting`. This is consistently used throughout the metadata-view related files in the codebase.
Applied to files:
src/elements/content-explorer/MetadataViewContainer.tsxsrc/elements/content-explorer/ContentExplorer.tsx
📚 Learning: 2025-08-28T15:38:35.075Z
Learnt from: greg-in-a-box
PR: box/box-ui-elements#4235
File: src/elements/content-explorer/MetadataViewContainer.tsx:106-123
Timestamp: 2025-08-28T15:38:35.075Z
Learning: In the box/metadata-view Column interface, the correct property name for enabling sorting is `allowsSorting`, not `allowSorting`.
Applied to files:
src/elements/content-explorer/MetadataViewContainer.tsx
🧬 Code graph analysis (3)
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js (7)
metadataQueryAPIHelper(15-15)metadataInstanceId1(19-19)metadataQueryResponse(70-116)nextMarker(69-69)getSchemaByTemplateKeyFunc(191-191)queryMetadataFunc(192-192)mdQuery(207-218)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
src/features/metadata-based-view/__tests__/MetadataBasedItemList.test.js (1)
fieldsToShow(102-107)src/features/metadata-based-view/MetadataBasedItemList.js (2)
fieldToShow(247-247)fieldName(284-284)
src/elements/content-explorer/ContentExplorer.tsx (2)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
MetadataViewContainerProps(106-114)ExternalFilterValues(33-40)src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
MetadataQueryAPIHelper(54-453)
⏰ 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 (19)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
136-144: LGTM on allowsSorting usage.Property name matches the metadata-view API. Respect.
Also applies to: 183-191
src/elements/content-explorer/ContentExplorer.tsx (1)
221-222: Solid V2 wiring with correct helper/call signature.Union type on helper and 4-arg V2 fetch look right. I like your style.
Also applies to: 486-493
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (17)
13-13: Describe label update LGTM, sucka.
196-200: mdQuery now includes item name—solid.This aligns with verifyQueryFields behavior adding extension/permissions later.
229-235: Empty template fields case covered—nice.Great guard ensuring flattened output remains stable even without schema types.
236-250: Missing template field handled—good defensive test.
261-266: Undefined template schema path verified—tight.
287-299: Null/undefined entries short-circuit—good.Prevents wasted network calls. I pity the fool who skips these checks.
358-367: fetchMetadataQueryResults error path covered—thumbs up.
413-435: getMetadataQueryFields edge cases nailed.No metadata fields → empty array behavior is correct.
487-502: verifyQueryFields uses FIELD_ITEM_NAME—consistency wins.
515-526: Required fields enforced—add a quick permission check note.You verify permissions below; solid coverage.
748-759: Empty enum/multiSelect values case—LGTM.
761-773: Empty string array for string field—LGTM.
775-799: verifyQueryFields + filters integration—baseline case solid.
800-824: AND logic composition—good.Maybe also assert param names don’t collide when more filters are added later.
Add a new test ensuring uniqueness when existing query_params contain keys like arg_status_1.
826-853: Merging with existing query/query_params—clean.Left-to-right AND with proper parentheses—approved.
855-868: No filters leaves query untouched—perfect discipline.
11-11: Swap all leftover FIELD_NAME imports to FIELD_ITEM_NAME across the repo
I pity the fool who leaves stray FIELD_NAME imports—rg shows hits in src/utils/fields.js, src/utils/comparator.js, src/features/metadata-based-view, src/elements/content-sharing, src/elements/content-picker, and more; update them all to FIELD_ITEM_NAME.⛔ Skipped due to learnings
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.
greg-in-a-box
force-pushed
the
mq-helper
branch
from
3b80d43 to
5f6edc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/ContentExplorer.tsx (1)
425-441: Set results state immediately; don’t block UI on folder-name fetchDon’t make users wait for the name lookup to see results. First set
nextState, then updaterootNamewhen the call returns.const nextState = { currentCollection: { ...currentCollection, ...metadataQueryCollection, percentLoaded: 100, }, markers: cloneMarkers, metadataTemplate, }; - // if v2, fetch folder name and add to state - if (metadataQuery?.ancestor_folder_id && isFeatureEnabled(features, 'contentExplorer.metadataViewV2')) { - this.api.getFolderAPI().getFolderFields( - metadataQuery.ancestor_folder_id, - ({ name }) => { - this.setState({ - ...nextState, - rootName: name || '', - }); - }, - this.errorCallback, - { fields: [FIELD_NAME] }, - ); - } else { - // No folder name to fetch, update state immediately with just metadata - this.setState(nextState); - } + // Update results immediately + this.setState(nextState); + // Fetch folder name in background (v2) + if (metadataQuery?.ancestor_folder_id && isFeatureEnabled(features, 'contentExplorer.metadataViewV2')) { + this.api.getFolderAPI().getFolderFields( + metadataQuery.ancestor_folder_id, + ({ name }) => this.setState({ rootName: name || '' }), + this.errorCallback, + { fields: [FIELD_NAME] }, + ); + }
♻️ Duplicate comments (8)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (4)
44-44: Avoid circular deps: import as type-only, fool.Use a type-only import for
ExternalFilterValuesto prevent runtime cycles.-import { ExternalFilterValues } from './MetadataViewContainer'; +import type { ExternalFilterValues } from './MetadataViewContainer';
425-434: Fix crash when query_params is missing and prevent arg key collisions.
customQueryParamscan be undefined → spreading inmergeQueryParamswill throw. Also seed arg indices from existing params to avoid placeholder reuse. I pity the fool who spreads undefined!- if (fields) { - const { query: filterQuery, queryParams: filteredQueryParams } = this.buildMDQueryParams(fields); - const { query: customQuery, query_params: customQueryParams } = clonedQuery; - const query = this.mergeQuery(customQuery, filterQuery); - const queryParams = mergeQueryParams(filteredQueryParams, customQueryParams); + if (fields) { + const { + query: customQuery, + query_params: customQueryParams = {}, + } = clonedQuery as typeof clonedQuery & { + query_params?: { [key: string]: number | Date | string }; + }; + const argIndexStart = Object.keys(customQueryParams).length; + const { query: filterQuery, queryParams: filteredQueryParams } = this.buildMDQueryParams( + fields, + argIndexStart, + ); + const query = this.mergeQuery(customQuery, filterQuery); + const queryParams = mergeQueryParams(filteredQueryParams, customQueryParams); if (query) { clonedQuery.query = query; clonedQuery.query_params = queryParams; } }
330-400: Harden filter builder: handle scalars, equality fallback, and seed index.
- Accept a start index to avoid arg collisions.
- Treat string value as scalar or array.
- For date/float, fall back to equality when not a range.
- Simplify query join.
- buildMDQueryParams = (filters: ExternalFilterValues) => { - let argIndex = 0; + buildMDQueryParams = (filters: ExternalFilterValues | null, argIndexStart = 0) => { + let argIndex = argIndexStart; let queries: string[] = []; let queryParams: { [key: string]: number | Date | string } = {}; if (filters) { Object.keys(filters).forEach(key => { const filter = filters[key]; if (!filter) { return; } const { fieldType, value } = filter; switch (fieldType) { case 'date': case 'float': { if (typeof value === 'object' && value !== null && 'range' in value) { const result = getRangeFilter(value, key, argIndex); queryParams = mergeQueryParams(queryParams, result.queryParams); queries = mergeQueries(queries, result.queries); argIndex += result.keysGenerated; break; } + // Equality fallback for single values + if (value != null && value !== '') { + const result = getRangeFilter( + { range: { gt: value as any, lt: value as any } } as any, + key, + argIndex, + ); + queryParams = mergeQueryParams(queryParams, result.queryParams); + queries = mergeQueries(queries, result.queries); + argIndex += result.keysGenerated; + } break; } case 'enum': case 'multiSelect': { const arrayValue = Array.isArray(value) ? value.map(v => String(v)) : [String(value)]; let result; if (key === 'mimetype-filter') { result = getMimeTypeFilter(arrayValue, key, argIndex); } else { result = getSelectFilter(arrayValue, key, argIndex); } queryParams = mergeQueryParams(queryParams, result.queryParams); queries = mergeQueries(queries, result.queries); argIndex += result.keysGenerated; break; } case 'string': { - if (value && value[0]) { - const result = getStringFilter(value[0], key, argIndex); + const str = Array.isArray(value) ? value[0] : (value as any); + if (str) { + const result = getStringFilter(String(str), key, argIndex); queryParams = mergeQueryParams(queryParams, result.queryParams); queries = mergeQueries(queries, result.queries); argIndex += result.keysGenerated; } break; } default: break; } }); } - const query = queries.reduce((acc, curr, index) => { - if (index > 0) { - acc += ` AND ${curr}`; - } else { - acc = curr; - } - return acc; - }, ''); + const query = queries.join(' AND '); return { queryParams, query, }; };
402-411: Preserve boolean precedence when merging queries, fool.Wrap both sides with parentheses to avoid
OR/ANDsurprises.- // Merge queries with AND operator - return `${customQuery} AND ${filterQuery}`; + // Merge queries with AND operator; wrap to preserve precedence + return `(${customQuery}) AND (${filterQuery})`;src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
775-868: These tests will fail until helper defaults query_params and seeds argIndex.
verifyQueryFieldscurrently spreads possibly-undefinedquery_paramsand doesn’t seed arg indices; fix the helper per my suggestion or this will blow up. I pity the fool who ignores failing tests!src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
573-696: Add missing cases for string passthrough and item_name→item.name mapping.Round out
convertFilterValuesToExternalcoverage with primitive string and key remap tests. Don’t slack, fool!+ test('should handle string values without array wrapping', () => { + const internal = { 'search-filter': { fieldType: 'string' as const, value: 'search term' } }; + const result = convertFilterValuesToExternal(internal as any); + expect(result['search-filter'].value).toBe('search term'); + expect(result['search-filter'].fieldType).toBe('string'); + }); + + test('should map item_name to item.name', () => { + const internal = { item_name: { fieldType: 'string' as const, value: 'filename.txt' } }; + const result = convertFilterValuesToExternal(internal as any); + expect(result['item.name']).toBeDefined(); + expect(result['item.name'].value).toBe('filename.txt'); + expect((result as any).item_name).toBeUndefined(); + });src/elements/content-explorer/ContentExplorer.tsx (2)
144-147: Omit exclusions fixed correctly — good hustle, fool!The union of excluded prop names is now syntactically correct and does what you intend.
1740-1742: Reset marker pagination when filters change, sucka!New filters must start at page 0 with fresh markers to avoid stale results.
- filterMetadata = (fields: ExternalFilterValues) => { - this.setState({ metadataFilters: fields }, this.refreshCollection); - }; + filterMetadata = (fields: ExternalFilterValues) => { + this.setState( + { + metadataFilters: fields, + currentPageNumber: 0, + markers: [], + }, + this.refreshCollection, + ); + };
🧹 Nitpick comments (7)
src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts (1)
84-91: Add one more escape test, fool.Cover backslash escaping to guard regressions in
escapeValueforgetStringFilterwith inputs containing\.Example:
+ test('should escape backslashes', () => { + const result = getStringFilter(String.raw`foo\bar`, 'field_name', 0); + expect(result).toEqual({ + queryParams: { arg_field_name_1: String.raw`%foo\\bar%` }, + queries: ['(field_name ILIKE :arg_field_name_1)'], + keysGenerated: 1, + }); + });Also applies to: 295-308
src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
175-196: fieldsToShow filter: confirm intent to drop item. fields.*You skip
item.*for filter chips. If consumers expect item.name/extension chips, document it or add unit tests here. Don’t confuse your users, fool.src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
800-824: Add precedence test for merged queries.After wrapping with parentheses, assert
'(A) AND (B)'form to prevent regressions.+ test('should wrap merged queries in parentheses', () => { + const metadataQuery = { from: 'enterprise_1234.templateKey', fields: [FIELD_ITEM_NAME], query: 'A', query_params: {} }; + const filters = { status: { fieldType: 'enum' as const, value: ['active'] } }; + const result = metadataQueryAPIHelper.verifyQueryFields(metadataQuery as any, filters as any); + expect(result.query).toBe('(A) AND (status HASANY (:arg_status_1))'); + });src/elements/content-explorer/ContentExplorer.tsx (4)
486-493: Confirm helper method signatures line up across V1/V2You’re calling
fetchMetadataQueryResultswith 4 args via a union-typed helper. Make sure both helpers expose a compatible signature to keep TS happy.If not already done, extract a shared interface (e.g.,
MetadataQueryAPI) implemented by both helpers to avoid union-call pitfalls.Also applies to: 221-221
945-954: Also reset markers on sort while in metadata viewChanging sort implies a new query; start pagination fresh.
sort = (sortBy: SortBy | Key, sortDirection: SortDirection) => { const { currentCollection: { id }, view, }: State = this.state; if (id || view === VIEW_METADATA) { - this.setState({ sortBy, sortDirection }, this.refreshCollection); + this.setState( + { + sortBy, + sortDirection, + ...(view === VIEW_METADATA ? { currentPageNumber: 0, markers: [] } : {}), + }, + this.refreshCollection, + ); } };
105-105: Providers wrapper: avoid double-wrapping, foolMake sure
hasProvidersis clearly documented and has a sane default so host apps don’t accidentally nest providers.Consider defaulting
hasProvidersto true internally or detecting existing providers to no-op.Also applies to: 130-130, 1839-1840
1830-1832: Minor: simplify hasPrev logicPage > 0 always has a previous page; you don’t need to peek at markers for this.
- const hasPreviousMarker: boolean = currentPageNumber === 1 || !!markers[currentPageNumber - 1]; + const hasPreviousMarker: boolean = currentPageNumber > 0;
📜 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 (2)
i18n/en-US.propertiesis excluded by!i18n/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
package.json(2 hunks)src/elements/common/__mocks__/mockMetadata.ts(1 hunks)src/elements/common/messages.js(1 hunks)src/elements/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(13 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(7 hunks)src/elements/content-explorer/MetadataQueryBuilder.ts(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(7 hunks)src/elements/content-explorer/__tests__/Content.test.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(11 hunks)src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(6 hunks)src/elements/content-explorer/stories/MetadataView.stories.tsx(2 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- package.json
- src/elements/common/messages.js
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
- src/elements/common/mocks/mockMetadata.ts
- src/elements/content-explorer/stories/MetadataView.stories.tsx
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
- src/elements/content-explorer/Content.tsx
- src/elements/content-explorer/MetadataQueryBuilder.ts
- src/elements/content-explorer/MetadataViewContainer.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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/__tests__/MetadataQueryBuilder.test.tssrc/elements/content-explorer/MetadataQueryAPIHelper.tssrc/elements/content-explorer/__tests__/MetadataViewContainer.test.tsxsrc/elements/content-explorer/ContentExplorer.tsx
📚 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-28T15:38:35.075Z
Learnt from: greg-in-a-box
PR: box/box-ui-elements#4235
File: src/elements/content-explorer/MetadataViewContainer.tsx:106-123
Timestamp: 2025-08-28T15:38:35.075Z
Learning: In the box/metadata-view Column interface used in MetadataViewContainer.tsx, the correct property name for enabling sorting is `allowsSorting`, not `allowSorting`. This is consistently used throughout the metadata-view related files in the codebase.
Applied to files:
src/elements/content-explorer/ContentExplorer.tsx
🧬 Code graph analysis (5)
src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts (1)
src/elements/content-explorer/MetadataQueryBuilder.ts (6)
mergeQueryParams(20-25)mergeQueries(27-29)getStringFilter(38-47)getRangeFilter(53-95)getSelectFilter(97-126)getMimeTypeFilter(128-159)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
ExternalFilterValues(33-40)src/elements/content-explorer/MetadataQueryBuilder.ts (6)
getRangeFilter(53-95)mergeQueryParams(20-25)mergeQueries(27-29)getMimeTypeFilter(128-159)getSelectFilter(97-126)getStringFilter(38-47)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (3)
MetadataViewContainerProps(106-114)ExternalFilterValues(33-40)convertFilterValuesToExternal(81-99)
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js (6)
metadataInstanceId1(19-19)metadataQueryResponse(70-116)nextMarker(69-69)getSchemaByTemplateKeyFunc(191-191)queryMetadataFunc(192-192)mdQuery(207-218)
src/elements/content-explorer/ContentExplorer.tsx (2)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
MetadataViewContainerProps(106-114)ExternalFilterValues(33-40)src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
MetadataQueryAPIHelper(54-453)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (5)
src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts (1)
231-242: LGTM on mimetype special-case, sucka.Tests correctly assert the 'mimetype-filter' → 'item.extension' mapping. That preserves the required behavior. I pity the fool who breaks this mapping!
src/elements/content-explorer/__tests__/Content.test.tsx (1)
33-33: LGTM on new onMetadataFilter prop.Prop is mocked and wired without breaking existing tests. Good hustle, sucka.
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
112-120: LGTM: table and filter wiring look tight.Good assertions on dual “Name” buttons and basic render path. Keep it sharp, sucka.
src/elements/content-explorer/ContentExplorer.tsx (2)
477-485: Correct name sort mapping in v2 — that’s tight!Falling back to
FIELD_ITEM_NAMEwhen sorting byFIELD_NAMEin v2 is the right call.
451-454: Keep the mime-type filter mapping intact across the stackPer prior learnings, ensure
MetadataQueryBuilder.getSelectFilterstill maps'mimetype-filter'→'item.extension'so multi-select works.I can cross-check builder/tests if you want.
Also applies to: 487-493
greg-in-a-box
force-pushed
the
mq-helper
branch
from
5f6edc2 to
83e781d
Compare
greg-in-a-box
force-pushed
the
mq-helper
branch
from
83e781d to
8ae2c9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (2)
235-263: Initial filter values: add explicit item.name string case and make “All Filters 3” assertion less brittle.
- Add a test for initialFilterValues keyed by 'item.name' with fieldType: 'string' to exercise the string path and mapping expectations.
- Avoid hard-coding the exact accessible name “All Filters 3”.
- expect(screen.getByRole('button', { name: 'All Filters 3' })).toBeInTheDocument(); + const allFiltersBtn = screen.getByRole('button', { name: /All Filters/ }); + expect(allFiltersBtn).toHaveTextContent(/\b3\b/); expect(screen.getByRole('button', { name: /Industry/i })).toHaveTextContent(/\(1\)/); expect(screen.getByRole('button', { name: /Category/i })).toHaveTextContent(/\(2\)/); + + // Also cover string + item.name mapping path + // (ensures the Name chip reflects a single applied value) + // NOTE: We pass this via initialFilterValues below in a separate test.Add this new test right after this block:
+ test('should accept initialFilterValues keyed by item.name (string)', () => { + renderComponent({ + actionBarProps: { + initialFilterValues: { + 'item.name': { + fieldType: 'string' as const, + value: 'Acme', + }, + } as unknown as ExternalFilterValues, + }, + }); + expect(screen.getByRole('button', { name: /Name/i })).toHaveTextContent(/\(1\)/); + });
573-696: Add missing coverage: string passthrough and item_name → item.name mapping.These are important regression guards for ExternalFilterValues conversion. Don’t be a fool—lock them down.
describe('convertFilterValuesToExternal', () => { @@ test('should handle multiSelect values', () => { @@ expect(result['category-filter'].fieldType).toBe('multiSelect'); }); + + test('should handle string values without array wrapping', () => { + const internalFilters = { + 'search-filter': { + fieldType: 'string' as const, + value: 'search term', + }, + }; + const result = convertFilterValuesToExternal(internalFilters); + expect(result['search-filter'].value).toBe('search term'); + expect(result['search-filter'].fieldType).toBe('string'); + }); + + test('should map item_name to item.name', () => { + const internalFilters = { + item_name: { + fieldType: 'string' as const, + value: 'filename.txt', + }, + }; + const result = convertFilterValuesToExternal(internalFilters); + expect(result['item.name']).toBeDefined(); + expect(result['item.name'].value).toBe('filename.txt'); + // @ts-expect-error ensure legacy key is not present + expect(result['item_name']).toBeUndefined(); + }); });src/elements/content-explorer/Content.tsx (2)
57-70: Default the callback in destructuring, or feel my pity!Without a default, JSX attr
onMetadataFilter={onMetadataFilter}can beundefinedagainst a required prop inMetadataViewContainer.- onMetadataFilter, + onMetadataFilter = () => {},
40-45: Make onMetadataFilter a safe no-op, fool — child prop is required.You marked
onMetadataFilteroptional here but pass it toMetadataViewContainerwhere it’s required. Default it to a noop in the component args to avoid TS errors and runtime undefined calls.- onMetadataFilter?: (fields: ExternalFilterValues) => void; + onMetadataFilter?: (fields: ExternalFilterValues) => void;src/elements/content-explorer/ContentExplorer.tsx (1)
1740-1742: Reset marker pagination on filter submit, sucka.Applying new filters with stale
markers/currentPageNumberrisks pulling the wrong page for a new query. Reset them before refresh.- filterMetadata = (fields: ExternalFilterValues) => { - this.setState({ metadataFilters: fields }, this.refreshCollection); - }; + filterMetadata = (fields: ExternalFilterValues) => { + this.setState( + { + metadataFilters: fields, + currentPageNumber: 0, + markers: [], + }, + this.refreshCollection, + ); + };src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
372-381: Wrap merged queries in parens — don’t get played by OR precedence.
A OR B AND Cain’t the same as(A OR B) AND (C). Protect precedence.- mergeQuery = (customQuery: string, filterQuery: string): string => { + mergeQuery = (customQuery: string, filterQuery: string): string => { if (!customQuery) { return filterQuery; } if (!filterQuery) { return customQuery; } - // Merge queries with AND operator - return `${customQuery} AND ${filterQuery}`; + // Merge queries with AND operator; wrap to preserve precedence + return `(${customQuery}) AND (${filterQuery})`; };
300-370: Harden filter builder: handle scalars, empty values, and simpler join.Right now:
- date/float ignores non-range scalars
- string assumes array access
value[0]- arg index always starts at 0 (will collide when merging later)
- manual reduce for joining
Refactor this hunk to be robust, fool.
- buildMetadataQueryParams = (filters: ExternalFilterValues) => { - let argIndex = 0; + buildMetadataQueryParams = (filters: ExternalFilterValues | null, argIndexStart = 0) => { + let argIndex = argIndexStart; let queries: string[] = []; let queryParams: { [key: string]: number | Date | string } = {}; - if (filters) { + if (filters) { Object.keys(filters).forEach(key => { const filter = filters[key]; if (!filter) { return; } const { fieldType, value } = filter; switch (fieldType) { case 'date': case 'float': { - if (typeof value === 'object' && value !== null && 'range' in value) { + if (typeof value === 'object' && value !== null && 'range' in value) { const result = getRangeFilter(value, key, argIndex); queryParams = mergeQueryParams(queryParams, result.queryParams); queries = mergeQueries(queries, result.queries); argIndex += result.keysGenerated; break; } - break; + // Equality fallback when scalar provided + if (value != null) { + const result = getRangeFilter({ range: { gt: value as any, lt: value as any } } as any, key, argIndex); + queryParams = mergeQueryParams(queryParams, result.queryParams); + queries = mergeQueries(queries, result.queries); + argIndex += result.keysGenerated; + } + break; } case 'enum': case 'multiSelect': { - const arrayValue = Array.isArray(value) ? value.map(v => String(v)) : [String(value)]; + const arrayValue = Array.isArray(value) + ? value.filter(v => v !== '').map(v => String(v)) + : value == null || value === '' + ? [] + : [String(value)]; let result; if (key === 'mimetype-filter') { result = getMimeTypeFilter(arrayValue, key, argIndex); } else { result = getSelectFilter(arrayValue, key, argIndex); } queryParams = mergeQueryParams(queryParams, result.queryParams); queries = mergeQueries(queries, result.queries); argIndex += result.keysGenerated; break; } case 'string': { - if (value && value[0]) { - const result = getStringFilter(value[0], key, argIndex); + const str = Array.isArray(value) ? value.find(Boolean) : (value as any); + if (str) { + const result = getStringFilter(String(str), key, argIndex); queryParams = mergeQueryParams(queryParams, result.queryParams); queries = mergeQueries(queries, result.queries); argIndex += result.keysGenerated; } break; } default: break; } }); } - const query = queries.reduce((acc, curr, index) => { - if (index > 0) { - acc += ` AND ${curr}`; - } else { - acc = curr; - } - return acc; - }, ''); + const query = queries.join(' AND '); return { queryParams, query, }; };
🧹 Nitpick comments (6)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (5)
116-116: Harden the “two Name buttons” assertion to avoid flakiness, fool.ARIA can shift; disambiguate header vs chip.
- expect(screen.getAllByRole('button', { name: 'Name' })).toHaveLength(2); // One in filter bar, one in table header + expect(screen.getByRole('columnheader', { name: 'Name' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /Name/ })).toBeInTheDocument();
325-351: Minor: stabilize the single “Name” button assertion.If roles or labels shift, the count can wobble. Consider asserting existence plus absence of header variant instead of raw length.
- expect(screen.getAllByRole('button', { name: 'Name' })).toHaveLength(1); // Only the one added by component + expect(screen.getByRole('button', { name: 'Name' })).toBeInTheDocument();
528-571: Test name overpromises—only enum is exercised. Rename or also cover float, fool.Either rename to reflect enum-only, or add a price (float) submission step. Quick win: rename.
- test('should handle multiple field types in filter submission', async () => { + test('should handle enum field submission', async () => {
610-625: Duplicate range case—cover float equals instead.You already test float range above. Exercise equals to broaden confidence.
- test('should keep float values unchanged', () => { + test('should keep float equals value unchanged', () => { const internalFilters = { 'rating-filter': { fieldType: 'float' as const, - value: { range: { gt: 4.5, lt: 5.0 }, advancedFilterOption: 'range' }, + value: { equals: 4.5, advancedFilterOption: 'equals' }, }, }; const result = convertFilterValuesToExternal(internalFilters); - expect(result['rating-filter'].value).toEqual({ - range: { gt: 4.5, lt: 5.0 }, - advancedFilterOption: 'range', - }); + expect(result['rating-filter'].value).toEqual({ equals: 4.5, advancedFilterOption: 'equals' }); expect(result['rating-filter'].fieldType).toBe('float'); });
573-696: Add a dedicated test for ‘mimetype-filter’ → ‘item.extension’ mapping (in MetadataQueryBuilder).Per learnings, this special-case mapping must not regress. Add a unit test in MetadataQueryBuilder.test.ts to assert the translation. I pity the fool who ships a broken mimetype filter.
// src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts import { getSelectFilter } from '../MetadataQueryBuilder'; describe('MetadataQueryBuilder.getSelectFilter', () => { test('maps mimetype-filter to item.extension', () => { const filter = getSelectFilter('mimetype-filter', ['pdf', 'docx']); expect(filter.field).toBe('item.extension'); expect(filter.values).toEqual(['pdf', 'docx']); }); });src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
561-774: Add cases for scalar equals and param collisions, fool.
- Add a test where a date/float filter is a single scalar (e.g.,
year: { fieldType: 'float', value: 2021 }) to assert equality behavior.- Add a test where
metadataQuery.query_paramsis pre-populated to ensure generated:arg_*keys don’t collide.
📜 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)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
package.json(2 hunks)src/elements/common/__mocks__/mockMetadata.ts(1 hunks)src/elements/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(13 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(5 hunks)src/elements/content-explorer/MetadataQueryBuilder.ts(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(7 hunks)src/elements/content-explorer/__tests__/Content.test.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(11 hunks)src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(6 hunks)src/elements/content-explorer/stories/MetadataView.stories.tsx(2 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- package.json
- src/elements/content-explorer/tests/Content.test.tsx
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
- src/elements/content-explorer/tests/MetadataQueryBuilder.test.ts
- src/elements/common/mocks/mockMetadata.ts
- src/elements/content-explorer/stories/MetadataView.stories.tsx
- src/elements/content-explorer/MetadataQueryBuilder.ts
- src/elements/content-explorer/MetadataViewContainer.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 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/__tests__/MetadataViewContainer.test.tsxsrc/elements/content-explorer/Content.tsxsrc/elements/content-explorer/ContentExplorer.tsxsrc/elements/content-explorer/MetadataQueryAPIHelper.ts
📚 Learning: 2025-08-28T15:38:35.075Z
Learnt from: greg-in-a-box
PR: box/box-ui-elements#4235
File: src/elements/content-explorer/MetadataViewContainer.tsx:106-123
Timestamp: 2025-08-28T15:38:35.075Z
Learning: In the box/metadata-view Column interface used in MetadataViewContainer.tsx, the correct property name for enabling sorting is `allowsSorting`, not `allowSorting`. This is consistently used throughout the metadata-view related files in the codebase.
Applied to files:
src/elements/content-explorer/ContentExplorer.tsx
📚 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
🧬 Code graph analysis (5)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (3)
MetadataViewContainerProps(106-114)ExternalFilterValues(33-40)convertFilterValuesToExternal(81-99)
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js (7)
metadataQueryAPIHelper(15-15)metadataInstanceId1(19-19)metadataQueryResponse(70-116)nextMarker(69-69)getSchemaByTemplateKeyFunc(191-191)queryMetadataFunc(192-192)mdQuery(207-218)
src/elements/content-explorer/Content.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
MetadataViewContainerProps(106-114)ExternalFilterValues(33-40)
src/elements/content-explorer/ContentExplorer.tsx (2)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
MetadataViewContainerProps(106-114)ExternalFilterValues(33-40)src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
MetadataQueryAPIHelper(50-423)
src/elements/content-explorer/MetadataQueryAPIHelper.ts (2)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
ExternalFilterValues(33-40)src/elements/content-explorer/MetadataQueryBuilder.ts (6)
getRangeFilter(53-95)mergeQueryParams(20-25)mergeQueries(27-29)getMimeTypeFilter(128-159)getSelectFilter(97-126)getStringFilter(38-47)
⏰ 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 (13)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (6)
6-10: LGTM on public API imports, sucka!Importing MetadataViewContainer, its types, and convert utilities looks correct and matches the exposed API. I pity the fool who breaks export surfaces—this ain’t it.
14-27: Mocks/columns align with item.name and new field types.
- Using 'item.name' in mock items and as the column id is consistent.
- Added float (price) and multiSelect (category) fields improve coverage.
- onMetadataFilter is wired and mocked.
All solid, fool.Also applies to: 47-62, 84-85, 101-102
108-110: Good test hygiene: clear mocks before each.Keeps call count assertions reliable. Respect.
141-159: Multi-select submit flow assertions look tight.Verifies incremental selections and emitted arrays. Good moves.
161-199: Dual callbacks validated—nice.onMetadataFilter and onFilterSubmit are both exercised with correct payloads. Clean.
201-233: onMetadataFilter-only path covered.Great guard ensuring external submit handler is optional. No complaints, fool.
src/elements/content-explorer/Content.tsx (1)
92-101: LGTM on V2 wiring and prop forwarding.Passing
fieldsToShowand the filter callback down only in V2 path is consistent with the feature flag split.src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (2)
11-14: Good rename to FIELD_ITEM_NAME.Tests now align with the public constant change. No concerns.
Also applies to: 196-201
314-322: Solid negative-path coverage for API errors.Nice rejection assertions. Keep it up.
Also applies to: 358-367
src/elements/content-explorer/ContentExplorer.tsx (3)
477-485: Smart default sort mapping to item.name in V2.Switching
FIELD_NAMEtoFIELD_ITEM_NAMEonly for V2 avoids breaking other views. Nice.
486-493: V2 fetch signature usage looks correct.Calling
fetchMetadataQueryResults(query, success, error, metadataFilters)matches the helper’s API.
501-506: Non-V2 fetch kept on 3-arg signature — approved.You’re instantiating the V1 helper here, so omitting filters is correct.
src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
40-41: Type-only import to dodge circulars — aces.Using
import typeforExternalFilterValuesavoids runtime cycles. Keep it.
greg-in-a-box
force-pushed
the
mq-helper
branch
from
8ae2c9f to
1947059
Compare
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 (3)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
66-78: Fix initial-value conversion; arrays aren’t always enums, sucka.This assumes any array means enum. For string filters, arrays should become a string (e.g., first non-empty token).
Apply:
function transformInitialFilterValuesToInternal( publicValues?: ExternalFilterValues, ): Record<string, { value: MetadataFormFieldValue }> | undefined { if (!publicValues) return undefined; - return Object.entries(publicValues).reduce<Record<string, { value: MetadataFormFieldValue }>>( - (acc, [key, { value }]) => { - acc[key] = Array.isArray(value) ? { value: { enum: value } } : { value }; + return Object.entries(publicValues).reduce<Record<string, { value: MetadataFormFieldValue }>>( + (acc, [key, { value, fieldType }]) => { + if (Array.isArray(value)) { + if (fieldType === 'enum' || fieldType === 'multiSelect') { + acc[key] = { value: { enum: value } as any }; + } else if (fieldType === 'string') { + const first = value.find(v => typeof v === 'string' && v.trim().length > 0) ?? ''; + acc[key] = { value: first as any }; + } else { + acc[key] = { value: value as any }; + } + } else { + acc[key] = { value: value as any }; + } return acc; }, {}, ); }src/elements/content-explorer/ContentExplorer.tsx (2)
455-463: Quit mutating state, fool! Compute page marker without writing to this.state.markers.Directly assigning into markers mutates state and can bite you with stale renders.
Apply this diff:
- if (currentPageNumber === 0) { - // Preserve the marker as part of the original query - markers[currentPageNumber] = metadataQueryClone.marker; - } - - if (typeof markers[currentPageNumber] === 'string') { - // Set marker to the query to get next set of results - metadataQueryClone.marker = markers[currentPageNumber]; - } + // Compute a safe marker to use for this page without mutating state + const pageMarker = + typeof markers[currentPageNumber] === 'string' ? markers[currentPageNumber] : metadataQueryClone.marker; + if (typeof pageMarker === 'string') { + metadataQueryClone.marker = pageMarker; + }
1577-1585: Clamp marker pagination to avoid negative pages.Don’t let currentPageNumber go below 0.
Apply this diff:
- markerBasedPaginate = (newOffset: number) => { - const { currentPageNumber } = this.state; - this.setState( - { - currentPageNumber: currentPageNumber + newOffset, // newOffset could be negative - }, - this.refreshCollection, - ); - }; + markerBasedPaginate = (newOffset: number) => { + const { currentPageNumber } = this.state; + const nextPage = Math.max(0, currentPageNumber + newOffset); + this.setState({ currentPageNumber: nextPage }, this.refreshCollection); + };
♻️ Duplicate comments (6)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
80-98: Harden enum detection; don’t blow up on primitives, fool.
'enum' in valuewithout object-guard can throw on strings/numbers; primitives also get miscast toRangeType | FloatType.Apply this diff:
export function convertFilterValuesToExternal(fields: FilterValues): ExternalFilterValues { return Object.entries(fields).reduce<ExternalFilterValues>((acc, [key, field]) => { - const { value, options, fieldType } = field; - - // Transform the value based on its type - const transformedValue: ExternalMetadataFormFieldValue = - 'enum' in value && Array.isArray(value.enum) - ? value.enum // Convert enum type to string array - : (value as RangeType | FloatType); // Keep range/float objects as-is + const { value, options, fieldType } = field; + const isObject = typeof value === 'object' && value !== null; + const isEnum = isObject && 'enum' in (value as Record<string, unknown>) && Array.isArray((value as any).enum); + const transformedValue: ExternalMetadataFormFieldValue = isEnum + ? (value as any).enum + : (value as any); // pass through strings, numbers, and range/float objects - acc[key === ITEM_FILTER_NAME ? FIELD_ITEM_NAME : key] = { + acc[key === ITEM_FILTER_NAME ? FIELD_ITEM_NAME : key] = { options, fieldType, value: transformedValue, };
105-112: MakeonMetadataFilteroptional to avoid breaking consumers, fool.Mark prop optional and guard the call; preserves back-compat.
-export interface MetadataViewContainerProps extends Omit<MetadataViewProps, 'items' | 'actionBarProps'> { +export interface MetadataViewContainerProps extends Omit<MetadataViewProps, 'items' | 'actionBarProps'> { actionBarProps?: ActionBarProps; currentCollection: Collection; metadataTemplate: MetadataTemplate; - onMetadataFilter: (fields: ExternalFilterValues) => void; + onMetadataFilter?: (fields: ExternalFilterValues) => void; /* Internally controlled onSortChange prop for the MetadataView component. */ onSortChange?: (sortBy: Key, sortDirection: string) => void; } @@ - onMetadataFilter, + onMetadataFilter, @@ - onMetadataFilter(transformed); + onMetadataFilter?.(transformed); if (onFilterSubmit) { onFilterSubmit(transformed); }Also applies to: 119-119, 193-200
src/elements/content-explorer/MetadataQueryBuilder.ts (1)
140-147: Avoid.endsWithon non-strings, sucka.Coerce before calling; current code can throw if a non-string sneaks in.
- filterValue.map(value => { - currentArgIndex += 1; - // the item-type-selector is returning the extensions with the suffix 'Type', so we remove it for the query - return [ - generateArgKey(fieldKey, currentArgIndex), - String(value.endsWith('Type') ? value.slice(0, -4) : value), - ]; - }), + filterValue.map(value => { + currentArgIndex += 1; + const raw = String(value); + // remove trailing 'Type' produced by the selector + const normalized = raw.endsWith('Type') ? raw.slice(0, -4) : raw; + return [generateArgKey(fieldKey, currentArgIndex), normalized]; + }),src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (2)
398-521: Add missing tests for string passthrough and item_name → item.name mapping.You promised these, but they ain’t here. I pity the fool who skips edge cases!
Apply this diff at the end of the describe block before it closes:
test('should handle multiSelect values', () => { const internalFilters = { 'category-filter': { fieldType: 'multiSelect' as const, options: [ { key: 'tech', id: 'tech1' }, { key: 'finance', id: 'finance1' }, ], value: { enum: ['tech', 'finance'] }, }, }; const result = convertFilterValuesToExternal(internalFilters); expect(result['category-filter'].value).toEqual(['tech', 'finance']); expect(result['category-filter'].fieldType).toBe('multiSelect'); }); + + test('should handle string values without array wrapping', () => { + const internalFilters = { + 'search-filter': { + fieldType: 'string' as const, + value: 'search term', + }, + }; + + const result = convertFilterValuesToExternal(internalFilters); + + expect(result['search-filter'].value).toBe('search term'); + expect(result['search-filter'].fieldType).toBe('string'); + }); + + test('should map item_name to item.name', () => { + const internalFilters = { + item_name: { + fieldType: 'string' as const, + value: 'filename.txt', + }, + }; + + const result = convertFilterValuesToExternal(internalFilters); + + expect(result['item.name']).toBeDefined(); + expect(result['item.name'].value).toBe('filename.txt'); + expect((result as any).item_name).toBeUndefined(); + });Run this to verify the tests exist and pass the intended assertions:
#!/bin/bash set -euo pipefail rg -n -C2 -P "should handle string values without array wrapping|should map item_name to item\.name" src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx
235-253: Use correct external key and string passthrough for initial filters, fool!Initial external filters should use 'item.name' (not 'name') and accept plain strings without array wrapping. Fix the test to reflect the real API surface.
Apply this diff:
const initialFilterValues = { industry: { fieldType: 'enum' as const, value: ['tech'], }, price: { fieldType: 'float' as const, value: { range: { gt: 10, lt: 100 } }, }, - name: { - fieldType: 'string' as const, - value: ['search term'], - }, + 'item.name': { + fieldType: 'string' as const, + value: 'search term', + }, category: { fieldType: 'multiSelect' as const, value: ['category1', 'category2'], }, } as unknown as ExternalFilterValues;src/elements/content-explorer/ContentExplorer.tsx (1)
1740-1743: Reset marker-based pagination when filters change.New filters + old markers = wrong page. Reset page and markers on filter change.
Apply this diff:
- filterMetadata = (fields: ExternalFilterValues) => { - this.setState({ metadataFilters: fields }, this.refreshCollection); - }; + filterMetadata = (fields: ExternalFilterValues) => { + this.setState( + { + metadataFilters: fields, + currentPageNumber: 0, + markers: [], + }, + this.refreshCollection, + ); + };
🧹 Nitpick comments (4)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
152-186: Prevent duplicate item-name filter chips across legacy keys, fool.Recognize
item_name,item.name, andnameto avoid dupes.- // Check if item_name field already exists to avoid duplicates - const hasItemNameField = fields.some((field: MetadataTemplateField) => field.key === ITEM_FILTER_NAME); + const hasItemNameField = fields.some((field: MetadataTemplateField) => + ['item_name', 'item.name', 'name'].includes(field.key), + );src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
498-499: Use async query to reduce flake, fool.Prefer
await screen.findByRole('row', { name: /Child 2/i })over immediategetByRoleto avoid timing flakes.- expect(screen.getByRole('row', { name: /Child 2/i })).toBeInTheDocument(); + expect(await screen.findByRole('row', { name: /Child 2/i })).toBeInTheDocument();src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
561-773: Add edge cases: zero bounds and string numerics in range, fool.Strengthen coverage for
0,'0', and non-finite inputs to match builder guards.Want me to add tests asserting
(year >= :arg_year_1)with0and ignoringNaN/''?src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
353-396: Exercise float submission path too, sucka.This test claims “multiple field types” but only asserts enum. Add a float submission/assertion to prove end-to-end handling.
📜 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)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
package.json(2 hunks)src/elements/common/__mocks__/mockMetadata.ts(1 hunks)src/elements/content-explorer/Content.tsx(4 hunks)src/elements/content-explorer/ContentExplorer.tsx(13 hunks)src/elements/content-explorer/MetadataQueryAPIHelper.ts(5 hunks)src/elements/content-explorer/MetadataQueryBuilder.ts(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(7 hunks)src/elements/content-explorer/__tests__/Content.test.tsx(1 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts(11 hunks)src/elements/content-explorer/__tests__/MetadataQueryBuilder.test.ts(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(6 hunks)src/elements/content-explorer/stories/MetadataView.stories.tsx(2 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- package.json
- src/elements/content-explorer/tests/Content.test.tsx
- src/elements/common/mocks/mockMetadata.ts
- src/elements/content-explorer/tests/MetadataQueryBuilder.test.ts
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
- src/elements/content-explorer/MetadataQueryAPIHelper.ts
- src/elements/content-explorer/Content.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 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/__tests__/MetadataViewContainer.test.tsxsrc/elements/content-explorer/MetadataViewContainer.tsxsrc/elements/content-explorer/ContentExplorer.tsxsrc/elements/content-explorer/MetadataQueryBuilder.tssrc/elements/content-explorer/stories/MetadataView.stories.tsx
📚 Learning: 2025-08-28T15:38:35.075Z
Learnt from: greg-in-a-box
PR: box/box-ui-elements#4235
File: src/elements/content-explorer/MetadataViewContainer.tsx:106-123
Timestamp: 2025-08-28T15:38:35.075Z
Learning: In the box/metadata-view Column interface used in MetadataViewContainer.tsx, the correct property name for enabling sorting is `allowsSorting`, not `allowSorting`. This is consistently used throughout the metadata-view related files in the codebase.
Applied to files:
src/elements/content-explorer/MetadataViewContainer.tsxsrc/elements/content-explorer/ContentExplorer.tsx
📚 Learning: 2025-08-28T15:38:35.075Z
Learnt from: greg-in-a-box
PR: box/box-ui-elements#4235
File: src/elements/content-explorer/MetadataViewContainer.tsx:106-123
Timestamp: 2025-08-28T15:38:35.075Z
Learning: In the box/metadata-view Column interface, the correct property name for enabling sorting is `allowsSorting`, not `allowSorting`.
Applied to files:
src/elements/content-explorer/MetadataViewContainer.tsx
🧬 Code graph analysis (4)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (3)
MetadataViewContainerProps(105-112)ExternalFilterValues(32-39)convertFilterValuesToExternal(80-98)
src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
src/features/metadata-based-view/__tests__/MetadataQueryAPIHelper.test.js (7)
metadataQueryAPIHelper(15-15)metadataInstanceId1(19-19)metadataQueryResponse(70-116)nextMarker(69-69)getSchemaByTemplateKeyFunc(191-191)queryMetadataFunc(192-192)mdQuery(207-218)
src/elements/content-explorer/ContentExplorer.tsx (2)
src/elements/content-explorer/MetadataViewContainer.tsx (2)
MetadataViewContainerProps(105-112)ExternalFilterValues(32-39)src/elements/content-explorer/MetadataQueryAPIHelper.ts (1)
MetadataQueryAPIHelper(50-423)
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
ContentExplorerProps(110-170)
⏰ 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/elements/content-explorer/stories/MetadataView.stories.tsx (2)
38-39: Solid addition of stable column keys, fool.Adding
keyalongsideidmatches the metadata-view Column API and prevents list/key churn. LGTM.
29-30: Verify mock schema supports${metadataSourceFieldName}.number, sucka.If
mockSchema.fieldsdoesn’t includenumber, the POST handler may return data missing this field, causing empty cells or confusion in the story.Would you confirm
mockSchemacontains anumberfield or drop it frommetadataQuery.fieldsin this story?src/elements/content-explorer/MetadataQueryBuilder.ts (1)
36-46: Confirm MDQL escape semantics for ILIKE before shipping, fool.If MDQL doesn’t treat “\” as default escape, literal “%”/“_” matches may fail. Consider appending
ESCAPE '\\'to the predicate.Proposed (only if MDQL supports it):
- queries: [`(${fieldKey} ILIKE :${argKey})`], + queries: [`(${fieldKey} ILIKE :${argKey} ESCAPE '\\' )`],Please verify against Box MDQL docs or a live query console.
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
471-477: Good move narrowing props type in test setup, sucka.
Partial<ContentExplorerProps>makes the test args flexible and future-proof. Thumbs up.src/elements/content-explorer/__tests__/MetadataQueryAPIHelper.test.ts (1)
196-201: FIELD_NAME → FIELD_ITEM_NAME rename looks tight, sucka.Tests align with the new constant and the helper’s expectations. Approved.
src/elements/content-explorer/ContentExplorer.tsx (1)
144-147: Omit union fix looks good.Type exclusion is now correct. Keep it tight.
tjuanitas
left a comment
tjuanitas
left a comment
There was a problem hiding this comment.
this: #4235 (comment)
lgtm
4 participants
Added the ability to filter via the filter chips on the metadata-view
Summary by CodeRabbit
New Features
Chores