fix(metadata-view): Hide keyword and location filters #4260
Conversation
WalkthroughI pity the fool who misses it: adds predefinedFilterOptions to MetadataViewContainer disabling keyword and location predefined filters, updates content-explorer tests (shared Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(245,250,255)
participant U as User
participant MVC as MetadataViewContainer
participant MV as MetadataView
participant AB as ActionBar
end
U->>MVC: Open Content Explorer
MVC->>MV: Render with transformed actionBarProps (includes predefinedFilterOptions)
MV->>AB: Init action bar with predefinedFilterOptions
AB-->>AB: Disable KeywordSearchFilterGroup & LocationFilterGroup
U->>AB: Open Filters Panel
AB-->>U: Show metadata template filters only (predefined controls hidden)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 (
|
![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 (3)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (2)
12-15: Scope that scrollTo mock, sucka.Global
Element.prototypemutation can leak across suites. Prefer a scoped spy with restore.Apply this change near your test hooks:
-Object.defineProperty(Element.prototype, 'scrollTo', { - value: jest.fn(), - writable: true, -}); +let scrollToSpy: jest.SpyInstance; +beforeAll(() => { + scrollToSpy = jest.spyOn(Element.prototype as any, 'scrollTo').mockImplementation(() => {}); +}); +afterAll(() => { + scrollToSpy.mockRestore(); +});
531-548: Make assertions resilient to i18n, fool.Relying on placeholder text “Enter keywords” and a button name matching /location/i can break with message changes/locales. Prefer stable roles or testids.
Example tweaks:
- Target the keywords input via role rather than placeholder:
expect(within(dialog).queryByRole('textbox', { name: /keywords/i })).not.toBeInTheDocument();
- Or add/test against data-testid exposed by MetadataView for these predefined filters.
src/elements/content-explorer/MetadataViewContainer.tsx (1)
214-217: Are we forcing disablement or allowing override? Decide, fool.Right now you overwrite any consumer-provided
predefinedFilterOptions. If product intent is “always hide keyword & location,” this is fine. If not, merge to preserve other options while enforcing disablement.Merge approach:
- predefinedFilterOptions: { - [PredefinedFilterName.KeywordSearchFilterGroup]: { isDisabled: true }, - [PredefinedFilterName.LocationFilterGroup]: { isDisabled: true, triggerCallback: noop }, - }, + predefinedFilterOptions: { + ...(actionBarProps?.predefinedFilterOptions ?? {}), + [PredefinedFilterName.KeywordSearchFilterGroup]: { + ...(actionBarProps?.predefinedFilterOptions?.[PredefinedFilterName.KeywordSearchFilterGroup] ?? {}), + isDisabled: true, + }, + [PredefinedFilterName.LocationFilterGroup]: { + ...(actionBarProps?.predefinedFilterOptions?.[PredefinedFilterName.LocationFilterGroup] ?? {}), + isDisabled: true, + triggerCallback: () => {}, + }, + },Also, drop lodash’s
noopfor an inline arrow to trim a dep, sucka.-import noop from 'lodash/noop'; ... - [PredefinedFilterName.LocationFilterGroup]: { isDisabled: true, triggerCallback: noop }, + [PredefinedFilterName.LocationFilterGroup]: { isDisabled: true, triggerCallback: () => {} },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP (Model Context Protocol) integration is disabled
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(2 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-18T20:57:20.993Z
Learnt from: jpan-box
PR: box/box-ui-elements#4227
File: src/elements/common/sub-header/SubHeaderRight.tsx:0-0
Timestamp: 2025-08-18T20:57:20.993Z
Learning: In the SubHeaderRight component's bulk action menu (src/elements/common/sub-header/SubHeaderRight.tsx), the BulkItemActionMenu trigger should only be shown when there are actual bulk actions available (bulkItemActions exists and has length > 0), not when the actions array is empty.
Applied to files:
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx
📚 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.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 (7)
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx (1)
122-123: LGTM: userEvent init order is solid, fool.Initializing
userbefore creating the mock keeps the test clean and deterministic. I pity the flaky test that don’t do this!src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (5)
113-117: Shared user instance FTW.Moving to a persistent
usermakes multi-step interactions reliable. That’s tight.
155-160: Good switch to user.click.Consistent
userusage improves readability and event sequencing. No jibber-jabber.
194-196: Nice consistency on interactions.The enum-select flow reads clean and stays async-safe. Respect.
232-234: More of the same goodness.Uniform
userinteractions reduce flake. Keep it rolling.
392-395: Solid pattern repeat.Enum path tested cleanly with the shared
user. Slick.src/elements/content-explorer/MetadataViewContainer.tsx (1)
16-23: Imports look right, chump.Bringing in
PredefinedFilterNameand a noop util matches the new config needs.
greg-in-a-box
force-pushed
the
hide-filters
branch
2 times, most recently
from
217775c to
b22a3a3
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)
package.json (1)
306-306: Align peerDependency with the new API or risk consumer breakage.Code and tests use features from @box/metadata-view >= 0.48.6, but peerDependencies still allow ^0.48.1. Consumers could install 0.48.1 and blow up at runtime. Bump the peer to keep semver honest, fool.
Apply this diff:
- "@box/metadata-view": "^0.48.1", + "@box/metadata-view": "^0.48.6",
🧹 Nitpick comments (1)
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
535-537: Avoid hardcoding the metadata column ID; derive it from test vars.Tie the expectation to metadataFieldNamePrefix so fixture tweaks don’t frag this test, sucka.
Apply this diff:
- expect(mockOnSortChangeExternal).toHaveBeenCalledWith({ - column: 'metadata.enterprise_0.templateName.last_contacted_at', - direction: 'ascending', - }); + expect(mockOnSortChangeExternal).toHaveBeenCalledWith({ + column: `${metadataFieldNamePrefix}.last_contacted_at`, + direction: 'ascending', + });
📜 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 (5)
package.json(1 hunks)src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(3 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/elements/common/sub-header/tests/SubHeaderRight.test.tsx
- src/elements/content-explorer/MetadataViewContainer.tsx
- src/elements/content-explorer/tests/MetadataViewContainer.test.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 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
📚 Learning: 2025-08-28T15:38:35.122Z
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.122Z
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/__tests__/ContentExplorer.test.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 (2)
package.json (1)
139-139: Dev dep bump looks tight.Upgrading @box/metadata-view to ^0.48.6 in devDependencies matches the new predefined filter API. I pity the fool who ships tests without the right API!
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
522-523: Sorting assertions are solid and exercise both callbacks.Finding the header via role, clicking, then asserting internal 'ASC' and external 'ascending' directions is crisp. I pity the fool who forgets to test both pathways.
Also applies to: 528-532
greg-in-a-box
force-pushed
the
hide-filters
branch
from
b22a3a3 to
4df9050
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
522-536: Sorting callbacks updated correctly—nice hustle!Clicking “Last Contacted At” and asserting internal 'last_contacted_at' + external full column ID is spot-on.
If you want extra confidence, add a second click to assert descending:
- await userEvent.click(lastContactedAtHeader); + await userEvent.click(lastContactedAtHeader); + await userEvent.click(lastContactedAtHeader); + expect(mockOnSortChangeInternal).toHaveBeenLastCalledWith('last_contacted_at', 'DESC'); + expect(mockOnSortChangeExternal).toHaveBeenLastCalledWith({ + column: 'metadata.enterprise_0.templateName.last_contacted_at', + direction: 'descending', + });For consistency with other tests, consider using the shared user instance from test-utils (like in MetadataViewContainer tests). I pity the inconsistency, fool!
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (3)
12-16: Mocking scrollTo globally—make it configurable and clean it upDefine it as configurable and remove it after the suite to avoid leaks. Don’t let a stubborn mock punk other tests.
-Object.defineProperty(Element.prototype, 'scrollTo', { - value: jest.fn(), - writable: true, -}); +beforeAll(() => { + Object.defineProperty(Element.prototype, 'scrollTo', { + value: jest.fn(), + configurable: true, + writable: true, + }); +}); +afterAll(() => { + // Restore JSDOM default + // @ts-expect-error deleting test-only property + delete Element.prototype.scrollTo; +});
113-117: Persistent userEvent instance—good move; add typingLock in the type to help TS help you, sucka.
- let user; + let user: ReturnType<typeof userEvent>; beforeEach(() => { - user = userEvent(); + user = userEvent(); jest.clearAllMocks(); });
531-548: Predefined filters test nails the requirementsAsserting File Type present, keywords/location absent matches the product ask. Clean and mean.
If the side panel role changes, the dialog query might get mouthy. Consider scoping to a stable test id or container to avoid role churn.
📜 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 (5)
package.json(2 hunks)src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(3 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/elements/content-explorer/MetadataViewContainer.tsx
- src/elements/common/sub-header/tests/SubHeaderRight.test.tsx
- package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T15:38:35.122Z
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.122Z
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/__tests__/ContentExplorer.test.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 (2)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (2)
155-160: Switched to user.click with a shared instance—solidLess flakiness, clearer intent. I approve, fool.
Also applies to: 194-196, 232-234, 392-395
84-107: allowsSorting usage verified
I pity the fool who’d doubt it—no strayallowSortingprops found in .ts/.tsx files.
|
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks. You may have to fix your CI before adding the pull request to the queue again. |
greg-in-a-box
force-pushed
the
hide-filters
branch
from
b804241 to
b2ec0bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (6)
12-16: Make scrollTo mock configurable and clean it up, fool!Without
configurable: true, other tests can blow up with “Cannot redefine property: scrollTo,” and you risk leaking globals across files. Addconfigurable: trueand clean up after the suite.Object.defineProperty(Element.prototype, 'scrollTo', { value: jest.fn(), - writable: true, + writable: true, + configurable: true, });Add this teardown (outside the shown range) to avoid cross-test pollution:
afterAll(() => { // Restore jsdom defaults // eslint-disable-next-line @typescript-eslint/no-explicit-any delete (Element.prototype as any).scrollTo; });
155-160: De-flake menu interactions with findByRole, fool!The menu might open async. Use
findByRoleand reuse the handle.- await user.click(screen.getByRole('button', { name: /Contact Role/ })); - await user.click(within(screen.getByRole('menu')).getByRole('menuitemcheckbox', { name: 'Developer' })); + await user.click(screen.getByRole('button', { name: /Contact Role/ })); + const menu1 = await screen.findByRole('menu'); + await user.click(within(menu1).getByRole('menuitemcheckbox', { name: 'Developer' })); @@ - await user.click(screen.getByRole('button', { name: /Contact Role/ })); - await user.click(within(screen.getByRole('menu')).getByRole('menuitemcheckbox', { name: 'Marketing' })); + await user.click(screen.getByRole('button', { name: /Contact Role/ })); + const menu2 = await screen.findByRole('menu'); + await user.click(within(menu2).getByRole('menuitemcheckbox', { name: 'Marketing' }));
194-196: Same async menu fix here, chump!- await user.click(screen.getByRole('button', { name: /Status/ })); - await user.click(within(screen.getByRole('menu')).getByRole('menuitemcheckbox', { name: 'Active' })); + await user.click(screen.getByRole('button', { name: /Status/ })); + const menu = await screen.findByRole('menu'); + await user.click(within(menu).getByRole('menuitemcheckbox', { name: 'Active' }));
232-234: Async menu again — tighten it up, fool!- await user.click(screen.getByRole('button', { name: /Status/ })); - await user.click(within(screen.getByRole('menu')).getByRole('menuitemcheckbox', { name: 'Active' })); + await user.click(screen.getByRole('button', { name: /Status/ })); + const menu = await screen.findByRole('menu'); + await user.click(within(menu).getByRole('menuitemcheckbox', { name: 'Active' }));
392-395: One more menu — don’t let timing punk you, sucka!- await user.click(screen.getByRole('button', { name: /Status/ })); - await user.click(within(screen.getByRole('menu')).getByRole('menuitemcheckbox', { name: 'Active' })); + await user.click(screen.getByRole('button', { name: /Status/ })); + const menu = await screen.findByRole('menu'); + await user.click(within(menu).getByRole('menuitemcheckbox', { name: 'Active' }));
531-548: Stabilize predefined-filter assertions — don’t depend on brittle copy, fool!Use async dialog lookup and loosen the keyword check to avoid i18n/wording breaks.
- await user.click(screen.getByRole('button', { name: 'All Filters' })); - expect(screen.getByRole('dialog')).toBeInTheDocument(); + await user.click(screen.getByRole('button', { name: 'All Filters' })); + const dialog = await screen.findByRole('dialog'); - expect(within(screen.getByRole('dialog')).getByRole('button', { name: 'File Type' })).toBeInTheDocument(); + expect(within(dialog).getByRole('button', { name: 'File Type' })).toBeInTheDocument(); - expect(within(screen.getByRole('dialog')).queryByPlaceholderText('Enter keywords')).not.toBeInTheDocument(); - expect( - within(screen.getByRole('dialog')).queryByRole('button', { name: /location/i }), - ).not.toBeInTheDocument(); + expect(within(dialog).queryByPlaceholderText(/keyword/i)).not.toBeInTheDocument(); + expect(within(dialog).queryByRole('button', { name: /location/i })).not.toBeInTheDocument();
📜 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 (5)
package.json(2 hunks)src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx(1 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(3 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(1 hunks)src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/elements/common/sub-header/tests/SubHeaderRight.test.tsx
- package.json
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
- src/elements/content-explorer/MetadataViewContainer.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 (2)
src/elements/content-explorer/__tests__/MetadataViewContainer.test.tsx (2)
1-550: Overall: solid coverage of the new filter behavior — nice work!Tests clearly assert that keyword and location are hidden while template filters remain. With the flake-proofing and typings above, you’ll be tough to beat.
113-117: Explicitly type theuservariable
I pity the fool who leaves it untyped—uselet user: ReturnType<typeof userEvent>;to avoid an implicit
any. YouruserEvent()call already invokes.setup()under the hood.Likely an incorrect or invalid review comment.
3 participants
before
after
Summary by CodeRabbit
New Features
Tests
Chores