feat(metadata): enable multilevel taxonomy field filter in preview #4354
Conversation
WalkthroughRenamed metadata request parameters (query_text → query, ancestor_id → ancestor, only_selectable_options → Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Storybook Test
participant Client as getMetadataOptions
participant Server as Metadata API / MSW
Note over Test,Client: Test triggers taxonomy option load
Test->>Client: getMetadataOptions({ query, ancestor, level, limit, marker, onlySelectable })
Client->>Server: GET /metadata?query=<query>&ancestor=<ancestor>&level=<level>&limit=<limit>&marker=<marker>&only-selectable-options=<bool>
Server-->>Client: 200 OK (options)
Client-->>Test: options
Note right of Test: Test calls waitForLoadingToComplete(canvas) before UI interactions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-08-12T18:04:17.698ZApplied to files:
📚 Learning: 2025-08-21T19:19:44.584ZApplied to files:
⏰ 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)
🔇 Additional comments (2)
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. Comment |
jcamden
force-pushed
the
drydock-13973-fix-metadata-taxonomy-field-filter-in-preview
branch
from
260c5eb to
579eb07
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: 0
🧹 Nitpick comments (1)
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx (1)
93-100: Consider refactoring for consistency or document the difference.This story uses an inline loading wait pattern that differs from the
waitForLoadingToCompletehelper used in taxonomy stories. The key differences are:
- Uses
queryByRole(non-throwing) instead offindByRole- Only waits for loading absence, doesn't verify it appeared first
- Uses a 5-second timeout instead of 10 seconds
If the loading behavior here is different (e.g., loading might not appear at all, or is very brief), consider adding a comment explaining why. Otherwise, refactor to use the helper for consistency.
Apply this diff if the loading behavior is the same:
- // Wait for loading to complete - await waitFor( - async () => { - const loadingIndicator = canvas.queryByRole('status', { name: 'Loading' }); - expect(loadingIndicator).not.toBeInTheDocument(); - }, - { timeout: 5000 }, - ); + // Wait for loading to complete + await waitForLoadingToComplete(canvas);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (2)
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx (2)
51-62: LGTM! Loading helper implementation is sound.The helper correctly waits for the loading indicator to appear and then disappear. The 10-second timeout is conservative but appropriate for visual regression tests where consistency matters more than speed.
642-642: LGTM! Consistent loading waits added to taxonomy stories.The addition of
waitForLoadingToCompletecalls in all taxonomy-related stories ensures the taxonomy data is fully loaded before test interactions begin. This is the correct approach to handle the asynchronous loading introduced by the API parameter changes.Also applies to: 672-672, 693-693, 741-741
jcamden
force-pushed
the
drydock-13973-fix-metadata-taxonomy-field-filter-in-preview
branch
from
cf20277 to
cda78ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx (1)
51-62: LGTM! Well-documented helper for taxonomy loading.The helper correctly waits for the loading indicator to appear and then disappear. The 10-second timeout is appropriately documented and the implementation handles the async loading behavior properly.
If other test files encounter similar taxonomy loading patterns, consider extracting this to a shared test utilities module (e.g.,
test-utils/waitForLoading.ts).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (1)
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx (1)
633-634: LGTM! Consistent usage across all taxonomy tests.The helper is appropriately called at the beginning of each taxonomy-related test, ensuring the loading state completes before interactions begin. The consistent pattern across all four tests improves test reliability.
Also applies to: 663-664, 684-685, 732-733
jcamden
force-pushed
the
drydock-13973-fix-metadata-taxonomy-field-filter-in-preview
branch
from
61c18a1 to
cf0431c
Compare
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx
Outdated
Show resolved
Hide resolved
…abel, scope queries
…es after package upgrades
jcamden
force-pushed
the
drydock-13973-fix-metadata-taxonomy-field-filter-in-preview
branch
from
e486206 to
651df6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx (1)
51-52: Consider clarifying the comment.The comment "I'm not exactly sure why" is informal for production code. If the reason is known (e.g., async data fetching for taxonomy options), consider updating the comment to be more precise. If the root cause is unclear, this is acceptable as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
package.json(1 hunks)src/api/Metadata.js(1 hunks)src/api/__tests__/Metadata.test.js(1 hunks)src/elements/content-sidebar/stories/__mocks__/TaxonomyMocks.ts(3 hunks)src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/api/tests/Metadata.test.js
- src/elements/content-sidebar/stories/mocks/TaxonomyMocks.ts
- package.json
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4248
File: src/elements/content-explorer/MetadataViewContainer.tsx:30-44
Timestamp: 2025-08-27T15:25:53.253Z
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.
📚 Learning: 2025-08-27T17:03:48.322Z
Learnt from: bxie-box
Repo: box/box-ui-elements PR: 4250
File: src/features/classification/applied-by-ai-classification-reason/__tests__/AppliedByAiClassificationReason.test.tsx:44-51
Timestamp: 2025-08-27T17:03:48.322Z
Learning: In test files for bxie-box, prefer using queryByTestId over getByTestId when testing for expected elements that should always exist, as null access serves as validation for regressions or unexpected changes rather than masking issues with defensive assertions.
Applied to files:
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx
📚 Learning: 2025-08-25T16:19:22.007Z
Learnt from: greg-in-a-box
Repo: box/box-ui-elements PR: 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/api/Metadata.js
🪛 Biome (2.1.2)
src/api/Metadata.js
[error] 1358-1359: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (3)
src/api/Metadata.js (1)
1349-1365: Parameter renames align with updated Box Taxonomies API.The destructuring and parameter mapping correctly implement the API parameter updates:
query_text→query,ancestor_id→ancestor, andonly_selectable_options→'only-selectable-options'. The variable usage is consistent throughout the params object construction.src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx (2)
689-712: Inline test helper improves determinism but may warrant extraction.The
navigateToSapporoTreeItemhelper defined inside the play function makes the taxonomy navigation more reliable by traversing the tree structure step-by-step. However, a past review comment flagged concerns about "logic inside of a test."Since this helper is specific to
EditMultilevelTaxonomyand used twice within the same test, the inline definition is reasonable. If similar navigation logic is needed in other taxonomy tests, consider extracting it to a shared test utility.Based on learnings
51-62: Loading synchronization helper reduces test flakiness.The
waitForLoadingToCompletehelper appropriately addresses the async nature of taxonomy data loading by waiting for the loading indicator to appear and then disappear. The 10-second timeout is reasonable for API-dependent operations.
|
Hey @tjuanitas, I spent some time looking into this today, and I finally discovered that the test flakiness was related to my upgrade of @box/combobox-with-api. My understanding is that the newer version makes additional API requests when editing multi-level taxonomy which were not part of the original. Those real requests were failing out (due to CORS) but taking a while to do so. When the response came back, the component would re-render, sometimes causing flakiness. Well, either the calls were new or the re-renders on failure were new, I'm not sure which. I was able to handle it however by adding the failed calls to our taxonomies mock api. After making this change, the tests can actually function without awaiting the loading state disappearance (since it is basically instantaneous), but keeping that in there is more realistic. Technically, that loading state is there... When I went to push these updates, I noticed that somebody had merged a commit updating some packages, including metadata-editor. That means that my PR is now only for updating the combobox-with-api and updating the now deprecated taxonomies API param names. The other PR didn't update combobox-with-api, possibly because of the problems it caused for tests---or maybe they just didn't need to. Anyway, this should now be good to go :) |
jcamden
force-pushed
the
drydock-13973-fix-metadata-taxonomy-field-filter-in-preview
branch
from
651df6f to
277379d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx (2)
51-62: Consider clarifying the comment for future maintainers.The helper function is well-implemented and correctly addresses the test flakiness. However, based on the PR description, the loading delay is due to the upgraded
@box/combobox-with-apipackage making additional API requests. Consider updating the comment to reflect this understanding instead of expressing uncertainty.-// I'm not exactly sure why, but the metadata sidebar shows a loading indicator for several seconds with taxonomies -// This await is needed in order for taxonomy tests to pass. +// The upgraded @box/combobox-with-api makes additional API requests when editing taxonomy fields, +// causing a loading indicator to appear. This helper waits for loading to complete before proceeding with tests. const waitForLoadingToComplete = async (canvas: ReturnType<typeof within>) => {
679-724: The refactored navigation logic improves test stability.The
navigateToSapporoTreeItemhelper uses robust async queries and is appropriately reused to verify selection state before and after the user interaction. This addresses the test flakiness from the upgraded dependency.Given the past review comment about removing logic from tests and the complexity of the navigation helper, consider extracting
navigateToSapporoTreeItemto a shared test utilities file if similar tree navigation is needed in other tests. This would improve maintainability while keeping individual tests focused on their specific assertions. However, the current approach is acceptable since the helper is reused within the same test and keeps the navigation logic visible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4248
File: src/elements/content-explorer/MetadataViewContainer.tsx:30-44
Timestamp: 2025-08-27T15:25:53.253Z
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.
🧬 Code graph analysis (1)
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx (1)
src/elements/content-preview/stories/tests/ContentPreview-metadata-visual.stories.js (1)
editButton(144-144)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (3)
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx (3)
630-646: Good use of the loading synchronization helper.The addition of
waitForLoadingToCompleteensures the taxonomy data is fully loaded before the test proceeds, addressing the flakiness mentioned in the PR description.
660-677: Good use of the loading synchronization helper.Consistent with the multilevel taxonomy test, this properly waits for loading to complete before proceeding with assertions.
726-772: Good addition of loading synchronization.The test now waits for loading to complete before proceeding, consistent with the other taxonomy tests. The extra blank line on 732 provides minor visual separation.
jcamden
force-pushed
the
drydock-13973-fix-metadata-taxonomy-field-filter-in-preview
branch
from
6af4ad0 to
2b89a94
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4248
File: src/elements/content-explorer/MetadataViewContainer.tsx:30-44
Timestamp: 2025-08-27T15:25:53.253Z
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.
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/components/form-elements/draft-js-mention-selector/index.js:4-4
Timestamp: 2025-09-16T14:53:03.538Z
Learning: The createMentionSelectorState export in src/components/form-elements/draft-js-mention-selector/index.js was confirmed to be internal-only within the box-ui-elements package, with no external references or documentation, making the breaking rename to createMentionTimestampSelectorState safe.
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 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
📚 Learning: 2025-08-21T19:19:44.584Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4237
File: src/api/ZipDownload.ts:6-8
Timestamp: 2025-08-21T19:19:44.584Z
Learning: For Box TypeScript SDK integrations, jpan-box prefers deep imports (e.g., from box-typescript-sdk-gen/lib/schemas/...generated.d.ts.js) over defining local types, citing that Box v2 APIs don't typically have breaking changes, making the deep import approach more viable despite potential brittleness.
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
jcamden
force-pushed
the
drydock-13973-fix-metadata-taxonomy-field-filter-in-preview
branch
from
dc3d12a to
e1b5e1f
Compare
…filter-in-preview
jcamden
deleted the
drydock-13973-fix-metadata-taxonomy-field-filter-in-preview
branch
4 participants
Summary
Updated deprecated taxonomy API parameter names to support multilevel taxonomy field filtering in content preview.
Background
The Box Taxonomies API deprecated several parameter names. This broke the combobox "type-ahead" results for taxonomy fields. This commit updates the code to use the new parameter names, restoring the combobox results.
Changes
Parameter Name Updates
Updated the following deprecated parameters to their current names:
query_text→queryancestor_id→ancestoronly_selectable_options→only-selectable-optionsFiles Modified
src/api/Metadata.js: Updated parameter mapping in the options requestsrc/api/__tests__/Metadata.test.js: Updated test assertions to use new parameter namessrc/elements/content-sidebar/SidebarUtils.js: Added debug console.log statementssrc/elements/content-sidebar/stories/__mocks__/TaxonomyMocks.ts: Updated mock to use new parameter nameDependencies
Explicitly upgraded packages:
@box/metadata-editor:^0.122.12→^0.137.3@box/combobox-with-api:^0.34.9→^1.3.11Transitive dependency upgrades (from
@box/combobox-with-api):@box/tree:^0.45.4→^1.2.6Testing
Note: APIs are mocked in our tests and we don't perform contract testing of APIs in this repository, which is why this type of breaking change wasn't caught automatically.
Manual Testing: Due to monoproxy issues, testing was done using a local proxy from the internal Crooze repository as a workaround. This change was manually verified against the actual public API after the deprecation was rolled out again in pre-prod envs. (See screenshot below.)
Summary by CodeRabbit