feat(metadata-view): bulk custom actions #4227
Conversation
WalkthroughAdds a reusable BulkItemActionMenu component and forwards an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ContentExplorer
participant SubHeader
participant SubHeaderRight
participant BulkItemActionMenu
participant BulkActionHandler as BulkItemAction.onClick
User->>ContentExplorer: select item(s)
ContentExplorer->>SubHeader: render({ bulkItemActions, selectedItemIds })
SubHeader->>SubHeaderRight: render({ bulkItemActions, selectedItemIds })
alt feature-enabled & selection & actions exist
SubHeaderRight->>BulkItemActionMenu: render(actions, selectedItemIds)
User->>BulkItemActionMenu: open & choose action
BulkItemActionMenu->>BulkActionHandler: onClick(selectedItemIds)
else menu not shown
SubHeaderRight->>SubHeaderRight: render metadata Button only
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
jpan-box
force-pushed
the
mdv-bca
branch
4 times, most recently
from
0f0765d to
0ce592f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🔭 Outside diff range comments (1)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
74-82: Leaking unknown prop to MetadataView (potential TS error) — exclude from spreadbulkItemActionMenuProps is declared on MetadataViewContainerProps but is not consumed here and is included in the {...rest} spread into . If MetadataViewProps doesn’t declare this prop (it likely doesn’t), TypeScript will error and, at runtime, React will pass an unknown prop to a composite component.
Extract and omit the prop from the spread to avoid leaking unknown props.
Apply this diff:
-const MetadataViewContainer = ({ - actionBarProps, - columns, - currentCollection, - metadataTemplate, - ...rest -}: MetadataViewContainerProps) => { +const MetadataViewContainer = ({ + actionBarProps, + columns, + currentCollection, + metadataTemplate, + bulkItemActionMenuProps, // intentionally not used here yet + ...rest +}: MetadataViewContainerProps) => { @@ - return <MetadataView actionBarProps={transformedActionBarProps} columns={columns} items={items} {...rest} />; + return <MetadataView actionBarProps={transformedActionBarProps} columns={columns} items={items} {...rest} />;Follow-up: If this prop is intended for SubHeader/Content (as per the PR scope), wire it there instead of to MetadataView.
Also applies to: 128-129
🧹 Nitpick comments (6)
src/elements/content-explorer/MetadataViewContainer.tsx (3)
69-72: Doc comment claims a default “Download” action that isn’t implementedThe comment states a default action will always show, but this file neither provides nor injects one. Either implement the fallback or update the comment to reflect current behavior to avoid misleading API consumers.
Apply one of the following diffs:
Option A — update the comment to current behavior:
- /* Array of custom actions to show in the bulk item action menu. Customers may optionally provide this value, but the Element will always - show a default action of "Download" */ + /* Array of custom actions to show in the bulk item action menu. If omitted, no bulk action menu is rendered. */Option B — keep the promise, inject a default if none provided (pseudo-implementation here; actual wiring likely belongs where the menu is rendered):
export interface BulkItemActionMenuProps { actions: BulkItemAction[]; }And at the render site of the menu (SubHeaderRight or equivalent), ensure:
const actions = bulkItemActionMenuProps?.actions?.length ? bulkItemActionMenuProps.actions : [{ label: 'Download', onClick: /* default handler */ }];
29-33: Make action onClick async-friendlyBulk actions commonly involve async work (API calls). Loosen the type to allow returning a Promise.
Apply this diff:
export interface BulkItemAction { label: string; - onClick: (selectedItemIds: Selection) => void; + onClick: (selectedItemIds: Selection) => void | Promise<void>; }
29-37: Type location/layering: avoid “common” importing types from “content-explorer”Per PR notes, SubHeader imports BulkItemActionMenuProps from this container. That inverts layering (common → content-explorer) and invites cycles. Prefer a shared types module (e.g., src/elements/types/bulkActions.ts) consumed by both “common” and “content-explorer”.
If you’d like, I can propose a small refactor PR to extract these interfaces into a neutral location and update imports accordingly.
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx (1)
25-37: Add coverage for the new bulk actions dropdownRecommend adding a test that, when selectedItemIds is non-empty (or 'all') and bulkItemActionMenuProps are provided, the ellipsis trigger renders with the aria-label from messages and clicking an item calls its onClick with the expected Selection.
I can draft a test case if helpful.
src/elements/content-explorer/ContentExplorer.tsx (1)
1738-1740: Consider implementing the TODO for default download action.The TODO comment indicates that a default download action should be added so that
bulkItemActionMenuPropsalways exists. This would prevent potential runtime errors when the bulk action menu is accessed but no actions are configured.Would you like me to help implement the default download action for bulk items? This would ensure consistent behavior and prevent the need for null checks downstream.
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
131-132: Mock functions should be passed inline for proper test isolation.Using
fn()inline in the story args can cause issues with test isolation when multiple stories share the same mock. Each story should have its own mock instance.Consider moving the mock function declaration to the story level:
export const metadataViewV2WithBulkItemActionMenuCallsOnClick: Story = { - args: metadataViewV2WithBulkItemActionMenuProps, + args: { + ...metadataViewV2ElementProps, + metadataViewProps: { + columns, + tableProps: { + isSelectAllEnabled: true, + }, + bulkItemActionMenuProps: { + actions: [ + { + label: 'Download', + onClick: fn(), + icon: Download, + }, + ], + }, + }, + }, play: async ({ canvas, args }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
i18n/en-US.properties(1 hunks)src/elements/common/sub-header/SubHeader.tsx(5 hunks)src/elements/common/sub-header/SubHeaderRight.scss(1 hunks)src/elements/common/sub-header/SubHeaderRight.tsx(6 hunks)src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx(1 hunks)src/elements/common/sub-header/messages.ts(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(2 hunks)src/elements/content-explorer/MetadataViewContainer.tsx(3 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/elements/common/sub-header/SubHeader.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
BulkItemActionMenuProps(35-37)
src/elements/common/sub-header/SubHeaderRight.tsx (1)
src/elements/content-explorer/MetadataViewContainer.tsx (1)
BulkItemActionMenuProps(35-37)
⏰ 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 (4)
i18n/en-US.properties (1)
1827-1828: ARIA label addition for bulk actions dropdown — looks goodKey name, value, and placement are consistent with the messages id and usage. No issues from i18n/accessibility standpoint.
src/elements/common/sub-header/messages.ts (1)
4-8: New i18n message definition is correct and consistentId matches en-US.properties, defaultMessage is clear, and the description is precise. Good addition.
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx (1)
22-23: Selected item ids default added — OKSetting selectedItemIds to an empty Set aligns with react-aria Selection semantics and unblocks rendering paths.
src/elements/common/sub-header/SubHeaderRight.scss (1)
23-25: Verified — class is used; no change requiredThe selector is applied and the style exists. The only note is a spacing consistency choice (margin-right vs margin-left).
- src/elements/common/sub-header/SubHeaderRight.tsx:106 — DropdownMenu.Trigger className="be-sub-header-right-dropdown-trigger"
- src/elements/common/sub-header/SubHeaderRight.scss:23–25 — .be-sub-header-right-dropdown-trigger { margin-right: 7px; }
- Related: other controls use margin-left: 7px (lines with .be-btn-sort, .be-btn-add, .bdl-ViewModeChangeButton)
Keep margin-right if the layout needs it; change to margin-left only for consistency if it doesn't affect positioning.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
106-112: Prevent undefined being passed to SubHeader: defaultbulkItemActionMenuPropsto an empty configSubHeader intentionally treats
bulkItemActionMenuPropsas required (per product direction and retrieved learnings). Here,ContentExplorerPropsmakes it optional and forwards it directly, which can type-error or passundefinedat runtime.Harden the handoff by always supplying a default object.
Apply this diff:
interface ContentExplorerProps { @@ - bulkItemActionMenuProps?: BulkItemActionMenuProps; + bulkItemActionMenuProps?: BulkItemActionMenuProps; @@ selectedItemIds={this.state.selectedItemIds} title={title} - bulkItemActionMenuProps={bulkItemActionMenuProps} + bulkItemActionMenuProps={bulkItemActionMenuProps ?? { actions: [] }} />If you prefer strict alignment with SubHeader’s required prop, we can also make
bulkItemActionMenuPropsrequired here and set a default indefaultProps, but the above avoids a breaking change while maintaining type-safety downstream.Also applies to: 1771-1774
🧹 Nitpick comments (2)
src/elements/common/sub-header/BulkItemActionMenu.tsx (1)
38-45: Use a stable key instead oflabelto avoid duplicate-key issuesUsing
labelas the React key can produce collisions if two actions share the same label, causing reconciliation bugs. Prefer a stable identifier.Two options:
- Add an
idfield toBulkItemActionand use it as the key.- Fallback to index if no id is available.
Example minimal change (adds
idas optional and prefers it as key):export interface BulkItemAction { + id?: string; label: string; onClick: (selectedItemIds: Selection) => void; } @@ - {actions.map(({ label, onClick }) => { + {actions.map(({ id, label, onClick }, index) => { return ( - <DropdownMenu.Item key={label} onSelect={() => onClick(selectedItemIds)}> + <DropdownMenu.Item key={id ?? `${label}-${index}`} onSelect={() => onClick(selectedItemIds)}> {label} </DropdownMenu.Item> ); })}src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
186-197: Story name says “Shows Ellipsis” but there’s no assertion — add one for intent clarityThe play function selects a row but doesn’t assert that the bulk actions trigger is present. Add an explicit check so the story exercises its stated behavior.
export const metadataViewV2WithBulkItemActionMenuShowsEllipsis: Story = { args: metadataViewV2WithBulkItemActionMenuProps, play: async ({ canvas }) => { await waitFor(() => { expect(canvas.getByRole('row', { name: /Child 2/i })).toBeInTheDocument(); }); const firstRow = canvas.getByRole('row', { name: /Child 2/i }); const checkbox = within(firstRow).getByRole('checkbox'); userEvent.click(checkbox); + + await waitFor(() => { + expect(canvas.getByRole('button', { name: 'Show actions for selected items' })).toBeInTheDocument(); + }); }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
i18n/en-US.properties(1 hunks)src/elements/common/sub-header/BulkItemActionMenu.tsx(1 hunks)src/elements/common/sub-header/SubHeader.tsx(5 hunks)src/elements/common/sub-header/SubHeaderRight.scss(1 hunks)src/elements/common/sub-header/SubHeaderRight.tsx(6 hunks)src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx(1 hunks)src/elements/common/sub-header/messages.ts(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(5 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/elements/common/sub-header/SubHeaderRight.scss
- i18n/en-US.properties
- src/elements/common/sub-header/messages.ts
- src/elements/common/sub-header/tests/SubHeaderRight.test.tsx
- src/elements/common/sub-header/SubHeaderRight.tsx
- src/elements/common/sub-header/SubHeader.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jpan-box
PR: box/box-ui-elements#4227
File: src/elements/common/sub-header/SubHeaderRight.tsx:104-125
Timestamp: 2025-08-15T14:42:09.910Z
Learning: In the SubHeaderRight component's bulk action menu (src/elements/common/sub-header/SubHeaderRight.tsx), the dropdown is intentionally shown even when bulkItemActionMenuProps has no actions, because Download will be made a default option available all the time in the future.
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.813Z
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.
📚 Learning: 2025-08-15T14:42:09.910Z
Learnt from: jpan-box
PR: box/box-ui-elements#4227
File: src/elements/common/sub-header/SubHeaderRight.tsx:104-125
Timestamp: 2025-08-15T14:42:09.910Z
Learning: In the SubHeaderRight component's bulk action menu (src/elements/common/sub-header/SubHeaderRight.tsx), the dropdown is intentionally shown even when bulkItemActionMenuProps has no actions, because Download will be made a default option available all the time in the future.
Applied to files:
src/elements/common/sub-header/BulkItemActionMenu.tsxsrc/elements/content-explorer/ContentExplorer.tsxsrc/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
📚 Learning: 2025-08-15T14:42:01.813Z
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.813Z
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/common/sub-header/BulkItemActionMenu.tsxsrc/elements/content-explorer/ContentExplorer.tsxsrc/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
🧬 Code Graph Analysis (1)
src/elements/content-explorer/ContentExplorer.tsx (1)
src/elements/common/sub-header/BulkItemActionMenu.tsx (1)
BulkItemActionMenuProps(16-18)
⏰ 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
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
Outdated
Show resolved
Hide resolved
jpan-box
force-pushed
the
mdv-bca
branch
2 times, most recently
from
11651a5 to
74005fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/elements/common/sub-header/SubHeaderRight.scss (3)
22-25: Potential double-spacing: use start-margin for consistency with siblingsSiblings use margin-left: 7px; adding margin-right here can create 14px of space between this trigger and the next control (7px right + 7px left). Also hurts RTL consistency.
Apply this minimal fix to align with existing pattern:
- .be-SubHeaderRight-bulkItemActionMenuDropdownTrigger { - margin-right: 7px; - } + .be-SubHeaderRight-bulkItemActionMenuDropdownTrigger { + margin-left: 7px; + }
22-25: Prefer logical properties or flex gap for RTL-safe, uniform spacingIf feasible, prefer margin-inline-start (or a container gap) to avoid hardcoding physical left/right and to make RTL support trivial.
Option A — logical margin (localized change):
- .be-SubHeaderRight-bulkItemActionMenuDropdownTrigger { - margin-right: 7px; - } + .be-SubHeaderRight-bulkItemActionMenuDropdownTrigger { + margin-inline-start: 7px; + }Option B — container-level gap (requires touching siblings outside this hunk): set gap on the flex container and remove per-child margins.
In this file outside the selected lines:
- Add to .be-sub-header-right: gap: 7px;
- Remove margin-left from .be-btn-sort, .be-btn-add, .bdl-ViewModeChangeButton, and this new trigger.
Example:
.be-sub-header-right {
display: flex;
align-items: center;
gap: 7px;
}
22-25: Class naming consistencyInternal classes here are mostly kebab-case (e.g., .be-btn-sort), while this introduces a long mixed-case class. If this selector mirrors a component-provided class, all good; otherwise consider a kebab-case variant for consistency (non-blocking).
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (4)
99-127: Per-item inline actions: icons/submenu are fine; consider fn() instead of noop for future assertionsCurrent usage is valid since the “no icons” rule applies only to bulk actions. To make it easier to assert per-item action invocations later, consider using fn() instead of noop.
Apply this diff to swap noop for fn():
actions: [ { label: 'Download', - onClick: noop, + onClick: fn(), icon: Download, }, ], @@ subMenuActions: [ { label: 'Request Signature', - onClick: noop, + onClick: fn(), icon: SignMeOthers, }, ],
185-196: Assert the ellipsis appears (and await clicks) to match the story’s intentThe story name promises visibility of the bulk ellipsis, but there’s no assertion. Also, awaiting userEvent.click reduces flakiness.
Apply this diff:
export const metadataViewV2WithBulkItemActionMenuShowsEllipsis: Story = { args: metadataViewV2WithBulkItemActionMenuProps, play: async ({ canvas }) => { await waitFor(() => { expect(canvas.getByRole('row', { name: /Child 2/i })).toBeInTheDocument(); }); const firstRow = canvas.getByRole('row', { name: /Child 2/i }); const checkbox = within(firstRow).getByRole('checkbox'); - userEvent.click(checkbox); + await userEvent.click(checkbox); + + await waitFor(() => { + expect(canvas.getByRole('button', { name: 'Show actions for selected items' })).toBeInTheDocument(); + }); }, };
198-221: Await interactions to avoid test flakinessAwaiting userEvent.click for the checkbox and the ellipsis button yields more deterministic tests.
Apply this diff:
const firstRow = canvas.getByRole('row', { name: /Child 2/i }); const checkbox = within(firstRow).getByRole('checkbox'); - userEvent.click(checkbox); + await userEvent.click(checkbox); await waitFor(() => { expect(canvas.getByRole('button', { name: 'Show actions for selected items' })).toBeInTheDocument(); }); const ellipsisButton = canvas.getByRole('button', { name: 'Show actions for selected items' }); - userEvent.click(ellipsisButton); + await userEvent.click(ellipsisButton);
223-252: Strengthen the onClick test: await interactions and verify callback receives selected IDs
- Await each click to reduce timing issues.
- Verify that the handler receives an array of selected IDs (length 1 in this scenario), aligning with the PR’s contract.
Apply this diff:
export const metadataViewV2WithBulkItemActionMenuCallsOnClick: Story = { args: metadataViewV2WithBulkItemActionMenuProps, play: async ({ canvas, args }) => { await waitFor(() => { expect(canvas.getByRole('row', { name: /Child 2/i })).toBeInTheDocument(); }); const firstRow = canvas.getByRole('row', { name: /Child 2/i }); const checkbox = within(firstRow).getByRole('checkbox'); - userEvent.click(checkbox); + await userEvent.click(checkbox); await waitFor(() => { expect(canvas.getByRole('button', { name: 'Show actions for selected items' })).toBeInTheDocument(); }); const ellipsisButton = canvas.getByRole('button', { name: 'Show actions for selected items' }); - userEvent.click(ellipsisButton); + await userEvent.click(ellipsisButton); await waitFor(() => { expect(screen.getByRole('menuitem', { name: 'Download' })).toBeInTheDocument(); }); const downloadAction = screen.getByRole('menuitem', { name: 'Download' }); - userEvent.click(downloadAction); + await userEvent.click(downloadAction); await waitFor(() => { expect(args.bulkItemActionMenuProps.actions[0].onClick).toHaveBeenCalled(); }); + + // Verify the handler received a single-element array of selected IDs + await waitFor(() => { + const calls = (args.bulkItemActionMenuProps.actions[0].onClick as any).mock.calls; + expect(calls.length).toBeGreaterThan(0); + const [selectedIds] = calls[0]; + expect(Array.isArray(selectedIds)).toBe(true); + expect(selectedIds).toHaveLength(1); + }); }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/elements/common/sub-header/BulkItemActionMenu.tsx(1 hunks)src/elements/common/sub-header/SubHeader.tsx(5 hunks)src/elements/common/sub-header/SubHeaderRight.scss(1 hunks)src/elements/common/sub-header/SubHeaderRight.tsx(6 hunks)src/elements/content-explorer/ContentExplorer.tsx(5 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/elements/common/sub-header/SubHeader.tsx
- src/elements/common/sub-header/BulkItemActionMenu.tsx
- src/elements/content-explorer/ContentExplorer.tsx
- src/elements/common/sub-header/SubHeaderRight.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jpan-box
PR: box/box-ui-elements#4227
File: src/elements/common/sub-header/SubHeaderRight.tsx:104-125
Timestamp: 2025-08-15T14:42:09.910Z
Learning: In the SubHeaderRight component's bulk action menu (src/elements/common/sub-header/SubHeaderRight.tsx), the dropdown is intentionally shown even when bulkItemActionMenuProps has no actions, because Download will be made a default option available all the time in the future.
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.813Z
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.
📚 Learning: 2025-08-15T14:42:09.910Z
Learnt from: jpan-box
PR: box/box-ui-elements#4227
File: src/elements/common/sub-header/SubHeaderRight.tsx:104-125
Timestamp: 2025-08-15T14:42:09.910Z
Learning: In the SubHeaderRight component's bulk action menu (src/elements/common/sub-header/SubHeaderRight.tsx), the dropdown is intentionally shown even when bulkItemActionMenuProps has no actions, because Download will be made a default option available all the time in the future.
Applied to files:
src/elements/common/sub-header/SubHeaderRight.scsssrc/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
📚 Learning: 2025-08-15T14:42:01.813Z
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.813Z
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/stories/tests/MetadataView-visual.stories.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 (4)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (4)
5-5: Storybook test utilities import looks goodUsing expect, fn, userEvent, waitFor, within, and screen from storybook/test is appropriate for play-function assertions and portal-rendered menus.
78-85: Bulk actions align with contract (no icons) and use a spy for onClickThanks for removing icons from bulk actions and wiring onClick with fn(); this matches the “no icons/subactions” assumption and avoids type errors noted earlier.
129-137: Correctly gating bulk menu behind Select AllSetting tableProps.isSelectAllEnabled = true for bulk-action scenarios matches the documented assumption.
279-280: Type alias move is finetype Story = StoryObj is the standard Storybook 7/8 pattern. Looks good.
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (5)
143-153: Await user interactions to reduce flakiness in the play functionAwaiting
userEventcalls is recommended for reliability in Storybook’s test runner.Apply this diff:
- userEvent.click(ellipsesButton); + await userEvent.click(ellipsesButton);
185-201: Stabilize the test by awaiting the click; optionally assert precondition
- Await the checkbox click to avoid race conditions.
- Optional: assert the bulk actions button is not present before selection to strengthen the test.
Apply this diff:
await waitFor(() => { expect(canvas.getByRole('row', { name: /Child 2/i })).toBeInTheDocument(); }); - const firstRow = canvas.getByRole('row', { name: /Child 2/i }); - const checkbox = within(firstRow).getByRole('checkbox'); - userEvent.click(checkbox); + const firstRow = canvas.getByRole('row', { name: /Child 2/i }); + const checkbox = within(firstRow).getByRole('checkbox'); + await userEvent.click(checkbox); await waitFor(() => { expect(canvas.getByRole('button', { name: 'Bulk actions' })).toBeInTheDocument(); });Optional strengthening (precondition check):
await waitFor(() => { expect(canvas.getByRole('row', { name: /Child 2/i })).toBeInTheDocument(); }); + // Not shown until at least one selection exists + expect(canvas.queryByRole('button', { name: 'Bulk actions' })).not.toBeInTheDocument();
202-226: Await interactions;screenusage here is correct due to portalled menus
- Await both checkbox and ellipsis clicks to reduce test flakiness.
- Keep using
screenfor the menu item lookup since the menu is likely portalled outside the canvas.Apply this diff:
const firstRow = canvas.getByRole('row', { name: /Child 2/i }); const checkbox = within(firstRow).getByRole('checkbox'); - userEvent.click(checkbox); + await userEvent.click(checkbox); await waitFor(() => { expect(canvas.getByRole('button', { name: 'Bulk actions' })).toBeInTheDocument(); }); const ellipsisButton = canvas.getByRole('button', { name: 'Bulk actions' }); - userEvent.click(ellipsisButton); + await userEvent.click(ellipsisButton); await waitFor(() => { expect(screen.getByRole('menuitem', { name: 'Download' })).toBeInTheDocument(); });
227-256: Await interactions and assert callback argument shape for stronger coverage
- Await all
userEvent.clickcalls.- In addition to “was called,” assert the callback received an array (selected IDs). This locks in the contract without relying on fixture-specific IDs.
Apply this diff:
const firstRow = canvas.getByRole('row', { name: /Child 2/i }); const checkbox = within(firstRow).getByRole('checkbox'); - userEvent.click(checkbox); + await userEvent.click(checkbox); await waitFor(() => { expect(canvas.getByRole('button', { name: 'Bulk actions' })).toBeInTheDocument(); }); const ellipsisButton = canvas.getByRole('button', { name: 'Bulk actions' }); - userEvent.click(ellipsisButton); + await userEvent.click(ellipsisButton); await waitFor(() => { expect(screen.getByRole('menuitem', { name: 'Download' })).toBeInTheDocument(); }); const downloadAction = screen.getByRole('menuitem', { name: 'Download' }); - userEvent.click(downloadAction); + await userEvent.click(downloadAction); await waitFor(() => { expect(args.bulkItemActionMenuProps.actions[0].onClick).toHaveBeenCalled(); + expect(args.bulkItemActionMenuProps.actions[0].onClick).toHaveBeenCalledWith(expect.any(Array)); });
129-137: Consider adding a story that verifies ellipsis shows even whenactionsis emptyPer the recorded learning, the dropdown should still show even if
bulkItemActionMenuProps.actionsis empty (preparing for a default Download). A small story will guard this behavior against regressions.Example story (add near the other bulk stories):
export const metadataViewV2WithEmptyBulkActionsStillShowsEllipsis: Story = { args: { ...metadataViewV2WithBulkItemActionMenuProps, bulkItemActionMenuProps: { actions: [] }, }, play: async ({ canvas }) => { await waitFor(() => { expect(canvas.getByRole('row', { name: /Child 2/i })).toBeInTheDocument(); }); const firstRow = canvas.getByRole('row', { name: /Child 2/i }); const checkbox = within(firstRow).getByRole('checkbox'); await userEvent.click(checkbox); await waitFor(() => { expect(canvas.getByRole('button', { name: 'Bulk actions' })).toBeInTheDocument(); }); }, };If you want, I can prep a patch adding this story and aligning all
userEventcalls across the file.Also applies to: 185-201
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
i18n/en-US.properties(1 hunks)src/elements/common/sub-header/BulkItemActionMenu.tsx(1 hunks)src/elements/common/sub-header/SubHeaderRight.scss(1 hunks)src/elements/common/sub-header/messages.ts(1 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/elements/common/sub-header/SubHeaderRight.scss
- i18n/en-US.properties
- src/elements/common/sub-header/messages.ts
- src/elements/common/sub-header/BulkItemActionMenu.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jpan-box
PR: box/box-ui-elements#4227
File: src/elements/common/sub-header/SubHeaderRight.tsx:104-125
Timestamp: 2025-08-15T14:42:09.910Z
Learning: In the SubHeaderRight component's bulk action menu (src/elements/common/sub-header/SubHeaderRight.tsx), the dropdown is intentionally shown even when bulkItemActionMenuProps has no actions, because Download will be made a default option available all the time in the future.
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.813Z
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.
📚 Learning: 2025-08-15T14:42:01.813Z
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.813Z
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/stories/tests/MetadataView-visual.stories.tsx
📚 Learning: 2025-08-15T14:42:09.910Z
Learnt from: jpan-box
PR: box/box-ui-elements#4227
File: src/elements/common/sub-header/SubHeaderRight.tsx:104-125
Timestamp: 2025-08-15T14:42:09.910Z
Learning: In the SubHeaderRight component's bulk action menu (src/elements/common/sub-header/SubHeaderRight.tsx), the dropdown is intentionally shown even when bulkItemActionMenuProps has no actions, because Download will be made a default option available all the time in the future.
Applied to files:
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.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 (5)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (5)
5-5: Importingscreenis appropriate for portalled menusGood call importing
screen; it’s needed to query portalled menu content outside the canvas. No issues here.
78-85: Bulk menu props align with the “no icons” contract and use a proper mockThe bulk action menu uses
fn()foronClickand omits icons, matching the stated assumptions and types. Looks good.
99-127: Inline item actions: icons and submenu usage are OK (bulk-only restriction doesn’t apply)Using
iconfor inline item actions and a submenu trigger is fine and consistent with the “no icons” restriction applying only to bulk actions.
129-137: Select-all enabled to surface bulk actionsSetting
tableProps.isSelectAllEnabled: truemeets the condition for showing the bulk actions control. Good wiring.
283-284: Story type alias looks goodSwitching to
type Story = StoryObj<typeof meta>at the bottom is clear and conventional. No concerns.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/elements/common/sub-header/BulkItemActionMenu.tsx (1)
25-25: Align className with SUIT CSS naming used in this repoUse the component’s PascalCase in the class for consistency with SUIT conventions previously noted in reviews.
Apply:
- <DropdownMenu.Trigger className="be-bulkItemActionMenu-trigger"> + <DropdownMenu.Trigger className="be-BulkItemActionMenu-trigger">Also update the corresponding selector in SubHeaderRight.scss to match:
.be-BulkItemActionMenu-trigger { margin-right: var(--space-2); }
🧹 Nitpick comments (3)
src/elements/common/sub-header/BulkItemActionMenu.tsx (3)
8-8: Use a simpler local import path for messagesMinor readability win; keep it relative to the current folder.
Apply:
-import messages from '../../common/sub-header/messages'; +import messages from './messages';
27-32: Remove redundant role="button" on a native ButtonButton already has implicit button semantics. Let DropdownMenu.Trigger manage aria-haspopup/aria-expanded. Removing the redundant role reduces potential a11y noise.
Apply:
<Button - role="button" aria-label={formatMessage(messages.bulkItemActionMenuAriaLabel)} icon={Ellipsis} size="large" variant="secondary" />
10-13: Use a stable id for React keys instead of labelUsing label as the key risks collisions and unstable reconciliation if labels change. Introduce an explicit id field and use it for keys.
Apply:
export interface BulkItemAction { + id: string; label: string; onClick: (selectedItemIds: Selection) => void; }- {actions.map(({ label, onClick }) => { + {actions.map(({ id, label, onClick }) => { return ( - <DropdownMenu.Item key={label} onSelect={() => onClick(selectedItemIds)}> + <DropdownMenu.Item key={id} onSelect={() => onClick(selectedItemIds)}> {label} </DropdownMenu.Item> ); })}Also applies to: 35-39
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
i18n/en-US.properties(1 hunks)src/elements/common/sub-header/BulkItemActionMenu.tsx(1 hunks)src/elements/common/sub-header/SubHeaderRight.scss(1 hunks)src/elements/common/sub-header/messages.ts(1 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/elements/common/sub-header/messages.ts
- src/elements/common/sub-header/SubHeaderRight.scss
- src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jpan-box
PR: box/box-ui-elements#4227
File: src/elements/common/sub-header/SubHeaderRight.tsx:104-125
Timestamp: 2025-08-15T14:42:09.910Z
Learning: In the SubHeaderRight component's bulk action menu (src/elements/common/sub-header/SubHeaderRight.tsx), the dropdown is intentionally shown even when bulkItemActionMenuProps has no actions, because Download will be made a default option available all the time in the future.
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.813Z
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.
📚 Learning: 2025-08-15T14:42:09.910Z
Learnt from: jpan-box
PR: box/box-ui-elements#4227
File: src/elements/common/sub-header/SubHeaderRight.tsx:104-125
Timestamp: 2025-08-15T14:42:09.910Z
Learning: In the SubHeaderRight component's bulk action menu (src/elements/common/sub-header/SubHeaderRight.tsx), the dropdown is intentionally shown even when bulkItemActionMenuProps has no actions, because Download will be made a default option available all the time in the future.
Applied to files:
src/elements/common/sub-header/BulkItemActionMenu.tsxi18n/en-US.properties
📚 Learning: 2025-08-15T14:42:01.813Z
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.813Z
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/common/sub-header/BulkItemActionMenu.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 (1)
i18n/en-US.properties (1)
1827-1828: ARIA label key added correctly and matches descriptor intentThe new key and comment align with the BulkItemActionMenu trigger’s accessibility needs.
✅ Verified exactly one occurrence in
i18n/en-US.properties.
⚠️ The key is currently missing in these locale files (potential l10n gaps):
- i18n/bn-IN.properties
- i18n/da-DK.properties
- i18n/de-DE.properties
- i18n/en-AU.properties
- i18n/en-CA.properties
- i18n/en-GB.properties
- i18n/en-x-pseudo.properties
- i18n/es-419.properties
- i18n/es-ES.properties
- i18n/fi-FI.properties
- i18n/fr-CA.properties
- i18n/fr-FR.properties
- i18n/hi-IN.properties
- i18n/it-IT.properties
- i18n/ja-JP.properties
- i18n/ko-KR.properties
- i18n/nb-NO.properties
- i18n/nl-NL.properties
- i18n/pl-PL.properties
- i18n/pt-BR.properties
- i18n/ru-RU.properties
- i18n/sv-SE.properties
- i18n/tr-TR.properties
- i18n/zh-CN.properties
- i18n/zh-TW.properties
Please add this key with the appropriate translations to those files or confirm that its omission is intentional.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/elements/common/sub-header/SubHeader.tsx (1)
18-42: Missing bulkItemActions forwarding to SubHeaderThe
bulkItemActionsprop is defined on bothContentExplorerPropsandSubHeaderProps, but it isn’t actually passed into the<SubHeader />inContentExplorer.tsx. Bulk actions won’t render unless this prop is threaded through.• In
src/elements/content-explorer/ContentExplorer.tsx(around line 1750), update the<SubHeader … />invocation to include:<SubHeader view={view} viewMode={viewMode} rootId={rootFolderId} isSmall={isSmall} rootName={rootName} currentCollection={currentCollection} canUpload={allowUpload} canCreateNewFolder={allowCreate} gridColumnCount={gridColumnCount} gridMaxColumns={GRID_VIEW_MAX_COLUMNS} gridMinColumns={GRID_VIEW_MIN_COLUMNS} maxGridColumnCountForWidth={maxGridColumnCount} onUpload={this.upload} onClearSelectedItemIds={this.clearSelectedItemIds} onCreate={this.createFolder} onGridViewSliderChange={this.onGridViewSliderChange} onItemClick={this.fetchFolder} onSortChange={this.sort} onViewModeChange={this.changeViewMode} portalElement={this.rootElement} + bulkItemActions={bulkItemActions} />• Ensure
bulkItemActionsis destructured (or accessed) from theContentExplorerprops and passed down.This change will restore the intended bulk‐action menu in the SubHeader.
🧹 Nitpick comments (3)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (2)
225-254: Also assert the selected IDs are passed to the bulk action callbackRight now the test only checks that onClick was called. Tighten the contract by asserting it’s called with a Set of selected IDs.
Apply this diff:
await waitFor(() => { expect(screen.getByRole('menuitem', { name: 'Download' })).toBeInTheDocument(); }); const downloadAction = screen.getByRole('menuitem', { name: 'Download' }); userEvent.click(downloadAction); await waitFor(() => { - expect(args.bulkItemActions[0].onClick).toHaveBeenCalled(); + expect(args.bulkItemActions[0].onClick).toHaveBeenCalled(); + // Ensure the handler receives the selected item ids + expect(args.bulkItemActions[0].onClick).toHaveBeenCalledWith(expect.any(Set)); });
121-135: Minor: consider using a named mock for clarityUsing fn() inline is fine. If you find yourself expanding assertions later, a named mock (e.g., const onDownload = fn()) improves readability and reuse across stories.
src/elements/common/sub-header/SubHeader.tsx (1)
18-20: Optional: default bulkItemActions to an empty array to avoid downstream nullability frictionSince SubHeaderRight forwards these directly to BulkItemActionMenu, providing a default here prevents accidental undefined from propagating when other consumers (e.g., ContentPicker) omit the prop.
Apply this diff:
-const SubHeader = ({ - bulkItemActions, +const SubHeader = ({ + bulkItemActions = [],Also applies to: 44-69, 102-123
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
i18n/en-US.properties(1 hunks)src/elements/common/sub-header/BulkItemActionMenu.tsx(1 hunks)src/elements/common/sub-header/SubHeader.tsx(5 hunks)src/elements/common/sub-header/SubHeaderRight.scss(1 hunks)src/elements/common/sub-header/SubHeaderRight.tsx(6 hunks)src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx(1 hunks)src/elements/common/sub-header/messages.ts(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(5 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/elements/common/sub-header/tests/SubHeaderRight.test.tsx
- src/elements/content-explorer/ContentExplorer.tsx
- src/elements/common/sub-header/SubHeaderRight.scss
- src/elements/common/sub-header/BulkItemActionMenu.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jpan-box
PR: box/box-ui-elements#4227
File: src/elements/common/sub-header/SubHeaderRight.tsx:104-125
Timestamp: 2025-08-15T14:42:09.910Z
Learning: In the SubHeaderRight component's bulk action menu (src/elements/common/sub-header/SubHeaderRight.tsx), the dropdown is intentionally shown even when bulkItemActionMenuProps has no actions, because Download will be made a default option available all the time in the future.
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.813Z
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.
📚 Learning: 2025-08-15T14:42:09.910Z
Learnt from: jpan-box
PR: box/box-ui-elements#4227
File: src/elements/common/sub-header/SubHeaderRight.tsx:104-125
Timestamp: 2025-08-15T14:42:09.910Z
Learning: In the SubHeaderRight component's bulk action menu (src/elements/common/sub-header/SubHeaderRight.tsx), the dropdown is intentionally shown even when bulkItemActionMenuProps has no actions, because Download will be made a default option available all the time in the future.
Applied to files:
src/elements/common/sub-header/messages.tsi18n/en-US.propertiessrc/elements/common/sub-header/SubHeader.tsxsrc/elements/common/sub-header/SubHeaderRight.tsxsrc/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
📚 Learning: 2025-08-15T14:42:01.813Z
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.813Z
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/common/sub-header/SubHeader.tsxsrc/elements/common/sub-header/SubHeaderRight.tsxsrc/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
🧬 Code Graph Analysis (2)
src/elements/common/sub-header/SubHeader.tsx (1)
src/elements/common/sub-header/BulkItemActionMenu.tsx (1)
BulkItemAction(10-13)
src/elements/common/sub-header/SubHeaderRight.tsx (1)
src/elements/common/sub-header/BulkItemActionMenu.tsx (2)
BulkItemAction(10-13)BulkItemActionMenu(20-45)
⏰ 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/stories/tests/MetadataView-visual.stories.tsx (2)
183-198: LGTM: good coverage for “shows bulk ellipsis only when a row is selected”The “select a row → see Bulk actions trigger” flow matches the stated assumptions and uses the ARIA label correctly.
200-223: LGTM: exercising menu open and item visibility via screen (portal-safe)Using screen for menu content is the right call since the menu likely portals; this avoids false negatives.
i18n/en-US.properties (1)
1827-1829: LGTM: ARIA label for bulk action trigger
- Key/value is correct and aligns with the message ID and existing tests.
- Verified that
boxui.subHeader.bulkItemActionMenuAriaLabelappears only once across alli18n/*.propertiesfiles—no duplicates found.src/elements/common/sub-header/messages.ts (1)
4-8: LGTM: message descriptor aligns with key and testsKey, defaultMessage, and description are consistent with en-US.properties and story queries.
src/elements/common/sub-header/SubHeaderRight.tsx (1)
23-41: Prop surface looks good and backward compatibleMaking bulkItemActions and selectedItemIds optional here keeps this component usable by ContentPicker without forcing bulk actions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/elements/common/sub-header/SubHeaderRight.tsx (3)
103-108: Confirm intent: bulk menu hidden unless actions exist — this diverges from prior stated behaviorCurrent condition requires
bulkItemActions.length > 0to show the bulk menu. Previously (per learnings), the trigger was intentionally shown even when empty to accommodate a default Download action later.If this change is intentional, ignore; otherwise, consider reverting to “show when there is a selection” and pass a safe empty array.
Apply this diff if you want to restore the previous behavior and keep it safe:
- {(selectedItemIds === 'all' || (selectedItemIds instanceof Set && selectedItemIds.size > 0)) && - bulkItemActions && - bulkItemActions.length > 0 && ( - <BulkItemActionMenu actions={bulkItemActions} selectedItemIds={selectedItemIds} /> - )} + {(selectedItemIds === 'all' || (selectedItemIds instanceof Set && selectedItemIds.size > 0)) && ( + <BulkItemActionMenu + actions={bulkItemActions ?? []} + selectedItemIds={selectedItemIds as Selection} + /> + )}
107-107: Type-safety: selectedItemIds is optional; BulkItemActionMenu expects SelectionEven with the guard, TS may not narrow
selectedItemIds?: Selectionto a concreteSelectionfor the JSX prop. Cast is sufficient here, since the condition guarantees presence.Apply this diff:
- <BulkItemActionMenu actions={bulkItemActions} selectedItemIds={selectedItemIds} /> + <BulkItemActionMenu + actions={bulkItemActions} + selectedItemIds={selectedItemIds as Selection} + />
24-24: API surface: optional props and feature gating
bulkItemActions?andselectedItemIds?are optional here. The product assumption states the trigger shows only if “Select All” is enabled. This file doesn’t know abouttableProps.isSelectAllEnabled, so the gating likely happens upstream. Please confirm upstream guarantees (e.g., passselectedItemIdsonly when bulk actions are enabled) to avoid accidental trigger visibility when selection is possible but Select All is disabled.If you prefer explicitness, we can add a boolean prop like
isBulkActionsEnabledto make the intent self-documenting and reduce coupling to upstream behavior.Also applies to: 38-38
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (4)
183-198: Stabilize interactions by awaiting userEvent.clickTo reduce flakiness in CI, await user interactions so event queues are flushed before assertions.
Apply this diff:
- userEvent.click(checkbox); + await userEvent.click(checkbox);
200-223: Good use of screen for portal content; consider awaiting userEvent.click and add a post-click assertionMenu content likely renders in a portal, so
screenis appropriate. Minor stability nit: await the click. Optionally assert focus or open state if available.Apply this diff:
- userEvent.click(ellipsisButton); + await userEvent.click(ellipsisButton);
225-254: Tighten assertion to validate payload shape; await clicks for stability
- Since you select a single row, the callback should receive a
Set(Selection) of 1 key. Asserting argument shape makes the test more valuable.- Also await the interaction steps.
Apply this diff:
- userEvent.click(checkbox); + await userEvent.click(checkbox); @@ - userEvent.click(ellipsisButton); + await userEvent.click(ellipsisButton); @@ - const downloadAction = screen.getByRole('menuitem', { name: 'Download' }); - userEvent.click(downloadAction); + const downloadAction = screen.getByRole('menuitem', { name: 'Download' }); + await userEvent.click(downloadAction); @@ - await waitFor(() => { - expect(args.bulkItemActions[0].onClick).toHaveBeenCalled(); - }); + await waitFor(() => { + expect(args.bulkItemActions[0].onClick).toHaveBeenCalledWith(expect.any(Set)); + });
121-136: Optional: add a negative story to verify Select All gatingTo guard the assumption “bulk icon only shows if Select All is enabled,” add a story with
isSelectAllEnabled: falsethat verifies the ellipsis doesn’t appear even after selecting an item.Here’s a ready-to-drop story you can add (outside this hunk):
export const metadataViewV2WithBulkItemActionMenuHiddenWhenSelectAllDisabled: Story = { args: { ...metadataViewV2ElementProps, bulkItemActions: [ { label: 'Download', onClick: fn() }, ], metadataViewProps: { columns, tableProps: { isSelectAllEnabled: false, }, }, }, play: async ({ canvas }) => { await waitFor(() => { expect(canvas.getByRole('row', { name: /Child 2/i })).toBeInTheDocument(); }); const firstRow = canvas.getByRole('row', { name: /Child 2/i }); const checkbox = within(firstRow).getByRole('checkbox'); await userEvent.click(checkbox); // Expect: bulk actions trigger should not be present await waitFor(() => { expect(screen.queryByRole('button', { name: 'Bulk actions' })).not.toBeInTheDocument(); }); }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/elements/common/sub-header/SubHeader.tsx(5 hunks)src/elements/common/sub-header/SubHeaderRight.tsx(6 hunks)src/elements/content-explorer/ContentExplorer.tsx(5 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/common/sub-header/SubHeader.tsx
- src/elements/content-explorer/ContentExplorer.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jpan-box
PR: box/box-ui-elements#4227
File: src/elements/common/sub-header/SubHeaderRight.tsx:104-125
Timestamp: 2025-08-15T14:42:09.910Z
Learning: In the SubHeaderRight component's bulk action menu (src/elements/common/sub-header/SubHeaderRight.tsx), the dropdown is intentionally shown even when bulkItemActionMenuProps has no actions, because Download will be made a default option available all the time in the future.
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.813Z
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.
📚 Learning: 2025-08-15T14:42:01.813Z
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.813Z
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/stories/tests/MetadataView-visual.stories.tsxsrc/elements/common/sub-header/SubHeaderRight.tsx
📚 Learning: 2025-08-15T14:42:09.910Z
Learnt from: jpan-box
PR: box/box-ui-elements#4227
File: src/elements/common/sub-header/SubHeaderRight.tsx:104-125
Timestamp: 2025-08-15T14:42:09.910Z
Learning: In the SubHeaderRight component's bulk action menu (src/elements/common/sub-header/SubHeaderRight.tsx), the dropdown is intentionally shown even when bulkItemActionMenuProps has no actions, because Download will be made a default option available all the time in the future.
Applied to files:
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsxsrc/elements/common/sub-header/SubHeaderRight.tsx
🧬 Code Graph Analysis (1)
src/elements/common/sub-header/SubHeaderRight.tsx (1)
src/elements/common/sub-header/BulkItemActionMenu.tsx (2)
BulkItemAction(10-13)BulkItemActionMenu(20-45)
⏰ 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 (4)
src/elements/common/sub-header/SubHeaderRight.tsx (2)
8-8: LGTM: centralizing bulk actions into a dedicated componentImporting
BulkItemActionandBulkItemActionMenuclarifies the API and keeps SubHeaderRight focused on orchestration. No issues found.
71-71: LGTM: clear, layered conditions (view type + feature flag)Gating on both
isMetadataViewandisMetadataViewV2Featurekeeps the legacy path untouched. Looks good.src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (2)
121-136: Story args for bulk actions look correct
bulkItemActionsusesfn()foronClickand no icons/sub-actions, matching the stated assumptions.tableProps.isSelectAllEnabled: truealigns with the “only show when Select All is enabled” assumption for these stories.
91-119: Item-level custom actions with icons are fine; separate from bulk-action “no icons” constraintThe icons and sub-actions here apply to per-item action menus, not the bulk action menu, so they don’t violate the “no icons in bulk” assumption.
tjuanitas
left a comment
tjuanitas
left a comment
There was a problem hiding this comment.
approving to unblock merge but i think these would be good improvements to quality
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
Outdated
Show resolved
Hide resolved
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
542-558: Scope the Download query to the opened menu to reduce flakinessQuerying the menu item through the menu container avoids accidental matches if other menus render in the DOM.
Apply this diff:
- await waitFor(() => { - expect(screen.getByRole('menuitem', { name: 'Download' })).toBeInTheDocument(); - }); - - const downloadAction = screen.getByRole('menuitem', { name: 'Download' }); + await waitFor(() => { + expect( + within(screen.getByRole('menu')).getByRole('menuitem', { name: 'Download' }), + ).toBeInTheDocument(); + }); + + const downloadAction = within(screen.getByRole('menu')).getByRole('menuitem', { + name: 'Download', + });Optional cleanup to simplify args extraction:
- const argsForFirstMockCall = mockOnClick.mock.calls[0]; - const firstArgToMockOnClick = argsForFirstMockCall[0]; + const [[firstArgToMockOnClick]] = mockOnClick.mock.calls;src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
208-231: Await user interactions and scope the menu query to improve reliabilityTwo clicks aren’t awaited and the menu-item query isn’t scoped. Awaiting userEvent reduces timing flake; scoping the query to the menu reduces false positives.
Apply this diff:
const firstRow = canvas.getByRole('row', { name: /Child 2/i }); const checkbox = within(firstRow).getByRole('checkbox'); - userEvent.click(checkbox); + await userEvent.click(checkbox); await waitFor(() => { expect(canvas.getByRole('button', { name: 'Bulk actions' })).toBeInTheDocument(); }); const ellipsisButton = canvas.getByRole('button', { name: 'Bulk actions' }); - userEvent.click(ellipsisButton); + await userEvent.click(ellipsisButton); - await waitFor(() => { - expect(screen.getByRole('menuitem', { name: 'Download' })).toBeInTheDocument(); - }); + await waitFor(() => { + expect( + within(screen.getByRole('menu')).getByRole('menuitem', { name: 'Download' }), + ).toBeInTheDocument(); + });Note: Keeping screen for the menu item is fine if the menu portals outside the canvas; scoping via within(screen.getByRole('menu')) reduces collisions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(1 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
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.
📚 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/content-explorer/stories/tests/MetadataView-visual.stories.tsx
📚 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/stories/tests/MetadataView-visual.stories.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__/ContentExplorer.test.tsx (1)
509-559: Solid RTL coverage for bulk bulk-item action flowWell structured: selects a row, surfaces the Bulk actions trigger, invokes the action, and asserts the onClick payload as a Set of IDs. Matches the contract and the feature assumptions (requires isSelectAllEnabled, works with single selection).
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
121-129: LGTM: bulk actions wiring follows the contract
- bulkItemActions provided with a single Download action and no icon/sub-actions (as per assumptions).
- tableProps.isSelectAllEnabled set, ensuring the trigger can render when selection is present.
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/elements/common/sub-header/SubHeaderRight.tsx (1)
73-74: Fix type narrowing for Selection; introduce a reusable type guard and remove redundant checksAccessing
.sizeonselectedItemIdswithout narrowing can trip TypeScript sinceSelectionis'all' | Set<Key>. You already do a proper runtime/type-safe check in Lines 107-111. Consolidate by introducing a type guard and using it both to computehasSelectedItemsand to gate the menu rendering. This also eliminates the duplicate selection logic.Apply this diff to use the guard:
- const hasSelectedItems: boolean = !!(selectedItemIds && (selectedItemIds === 'all' || selectedItemIds.size > 0)); + const hasSelectedItems = hasSelection(selectedItemIds);And update the inner render condition:
- {(selectedItemIds === 'all' || (selectedItemIds instanceof Set && selectedItemIds.size > 0)) && - bulkItemActions && - bulkItemActions.length > 0 && ( - <BulkItemActionMenu actions={bulkItemActions} selectedItemIds={selectedItemIds} /> - )} + {hasSelection(selectedItemIds) && bulkItemActions && bulkItemActions.length > 0 && ( + <BulkItemActionMenu actions={bulkItemActions} selectedItemIds={selectedItemIds} /> + )}Add this type guard near the top of the file (outside the diffed lines):
const hasSelection = (sel?: Selection): sel is Selection => sel === 'all' || (sel instanceof Set && sel.size > 0);src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
215-226: Await user interactions to avoid race conditions in VRTTwo clicks aren’t awaited. Storybook’s
userEventAPIs are async; addawaitfor stability and to prevent flaky screenshots.Apply this diff:
- const checkbox = within(firstRow).getByRole('checkbox'); - userEvent.click(checkbox); + const checkbox = within(firstRow).getByRole('checkbox'); + await userEvent.click(checkbox); - const ellipsisButton = canvas.getByRole('button', { name: 'Bulk actions' }); - - userEvent.click(ellipsisButton); + const ellipsisButton = canvas.getByRole('button', { name: 'Bulk actions' }); + await userEvent.click(ellipsisButton);src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx (2)
130-161: Great: verifies onClick payload for 'all' selectionThis asserts the callback receives
'all'when that mode is active. Consider also adding a case to ensure the trigger is not shown whenbulkItemActionsis empty/undefined to protect against regressions in the visibility rule.If you want, I can add a test like:
- “does not render Bulk actions when bulkItemActions is empty”
- “does not render Bulk actions when selectedItemIds is an empty Set”
163-179: Align test name with assertionsThis test hides both the bulk actions trigger and the metadata button when the feature is disabled. Rename to reflect both expectations.
Apply this diff:
- test('should not render metadata button when metadataViewV2 feature is disabled', async () => { + test('should hide Bulk actions and Metadata button when metadataViewV2 feature is disabled', async () => {src/elements/content-explorer/__tests__/ContentExplorer.test.tsx (1)
509-559: Tighten assertion: also assert single invocationSolid end-to-end coverage of the bulk action flow. Add
toHaveBeenCalledTimes(1)to make the expectation stricter and reduce false positives from accidental double-invocations.Apply this diff:
const expectedOnClickArgument = new Set(['1188890835']); await waitFor(() => { - expect(mockOnClick).toHaveBeenCalled(); + expect(mockOnClick).toHaveBeenCalled(); + expect(mockOnClick).toHaveBeenCalledTimes(1); // Array conversion from sets to avoid set comparison issues in Jest const argsForFirstMockCall = mockOnClick.mock.calls[0]; const firstArgToMockOnClick = argsForFirstMockCall[0]; expect(Array.from(firstArgToMockOnClick)).toEqual(Array.from(expectedOnClickArgument)); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/elements/common/sub-header/SubHeaderRight.tsx(5 hunks)src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx(3 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(1 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
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.
📚 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/content-explorer/stories/tests/MetadataView-visual.stories.tsxsrc/elements/common/sub-header/SubHeaderRight.tsx
📚 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/stories/tests/MetadataView-visual.stories.tsxsrc/elements/common/sub-header/SubHeaderRight.tsx
🧬 Code Graph Analysis (2)
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx (2)
src/elements/common/sub-header/SubHeaderRight.tsx (1)
SubHeaderRightProps(23-42)src/elements/content-explorer/ContentExplorer.tsx (1)
render(1691-1964)
src/elements/common/sub-header/SubHeaderRight.tsx (1)
src/elements/common/sub-header/BulkItemActionMenu.tsx (2)
BulkItemAction(10-13)BulkItemActionMenu(20-45)
⏰ 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/common/sub-header/SubHeaderRight.tsx (2)
105-116: Conditional render logic reads correctly and matches feature gatingThe grouping
isMetadataView && isMetadataViewV2Feature && hasSelectedItemsand the additional guard to only show the bulk menu when actions exist align with the “show trigger only when actions are available” learning. With the type guard refactor above, this block can stay logically unchanged while getting simpler types.
8-8: Import style is correctUsing
import { type BulkItemAction, BulkItemActionMenu }adheres to the preferred type-import style and matches prior feedback.src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (2)
121-135: OK to use fn() for non-assertive storyUsing
fn()here is fine since this story validates visual states and doesn’t assert calls. If you ever promote this to assert behavior, switch to a captured mock to make expectations explicit.
258-259: Type alias placement is fineDeclaring
type Story = StoryObj<typeof meta>at the bottom is acceptable; TS resolves types regardless of order, andmetais defined above. No change needed.src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx (1)
105-129: Positive: selection → bulk menu rendering is covered (both 'all' and Set)Good coverage ensuring the trigger appears for both
'all'and non-emptySetselections under the feature flag.
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx
Outdated
Show resolved
Hide resolved
tjuanitas
left a comment
tjuanitas
left a comment
There was a problem hiding this comment.
i had nits but it's fine from me
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-explorer/__tests__/ContentExplorer.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx
Outdated
Show resolved
Hide resolved
Refactor to use RTL
Refactor tests
Remove unnecessary awaits
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
272-273: Type aliasStoryis declared after first use; move it above story exports
Storyis referenced starting at Line 69. Type aliases aren’t hoisted; depending on TS config this can error. Move the alias near the top, independent ofmeta, to break the cycle.-type Story = StoryObj<typeof meta>; +// Moved near the top; see snippet below.Add this near the imports (above the first
export const ...: Story):// Place after imports so it's available to all story exports type Story = StoryObj<typeof ContentExplorer>;
🧹 Nitpick comments (5)
src/elements/common/sub-header/messages.ts (1)
4-8: Standardize “ARIA” capitalization in sub-header message descriptionMinor nit: update the description to capitalize “ARIA” consistently with accessibility terminology.
Location:
- File: src/elements/common/sub-header/messages.ts (lines 4–8)
Suggested diff:
bulkItemActionMenuAriaLabel: { defaultMessage: 'Bulk actions', - description: 'Aria-label for the dropdown menu that shows actions for selected items', + description: 'ARIA label for the dropdown menu that shows actions for selected items', id: 'boxui.subHeader.bulkItemActionMenuAriaLabel', },src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (4)
5-5: Prefer canvas-scoped queries; dropscreenimport once updated belowWe’ll switch the later assertions to canvas-scoped
findByRoleto avoid cross-story leakage and flakiness. Thenscreenis unused.-import { expect, fn, userEvent, waitFor, within, screen } from 'storybook/test'; +import { expect, fn, userEvent, waitFor, within } from 'storybook/test';
91-119: Inline custom actions configuration is clear and type-alignedUse of icons/submenu is appropriate for inline item actions (bulk actions remain iconless). Table
isSelectAllEnabledhere is harmless, though only required for bulk actions per your assumptions.If you want to tighten scope, you can omit
tablePropshere since it’s not needed for row-level inline menus.
121-136: Bulk actions story props meet the contract (no icons, Select All enabled)Looks correct. One small improvement: bind the mock so it’s assertable later if you add an interaction assertion.
- bulkItemActions: [ - { - label: 'Download', - onClick: fn(), - }, - ], + bulkItemActions: [ + { + label: 'Download', + onClick: fn(), // consider assigning to a const if you plan to assert calls + }, + ],
222-246: Await user events and prefer canvas-scopedfindByRoleto reduce flakinessTwo clicks aren’t awaited, and the last assertion mixes in
screen. Awaiting events and usingcanvas.findByRolesimplifies timing and scoping.export const metadataViewV2WithBulkItemActionMenuShowsItemActionMenu: Story = { args: metadataViewV2WithBulkItemActions, play: async ({ canvas }) => { await waitFor(() => { expect(canvas.getByRole('row', { name: /Child 2/i })).toBeInTheDocument(); }); const firstRow = canvas.getByRole('row', { name: /Child 2/i }); const checkbox = within(firstRow).getByRole('checkbox'); - userEvent.click(checkbox); + await userEvent.click(checkbox); - await waitFor(() => { - expect(canvas.getByRole('button', { name: 'Bulk actions' })).toBeInTheDocument(); - }); - - const ellipsisButton = canvas.getByRole('button', { name: 'Bulk actions' }); - - userEvent.click(ellipsisButton); - - await waitFor(() => { - expect(screen.getByRole('menuitem', { name: 'Download' })).toBeInTheDocument(); - }); + const bulkActionsButton = await canvas.findByRole('button', { name: 'Bulk actions' }); + await userEvent.click(bulkActionsButton); + await canvas.findByRole('menuitem', { name: 'Download' }); }, };After this change, the
screenimport can be removed (see comment on Line 5).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
i18n/en-US.properties(1 hunks)src/elements/common/sub-header/BulkItemActionMenu.tsx(1 hunks)src/elements/common/sub-header/SubHeader.tsx(4 hunks)src/elements/common/sub-header/SubHeaderRight.scss(1 hunks)src/elements/common/sub-header/SubHeaderRight.tsx(5 hunks)src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx(3 hunks)src/elements/common/sub-header/messages.ts(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(4 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(1 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/elements/common/sub-header/tests/SubHeaderRight.test.tsx
- src/elements/common/sub-header/SubHeaderRight.scss
- src/elements/common/sub-header/BulkItemActionMenu.tsx
- src/elements/content-explorer/ContentExplorer.tsx
- src/elements/common/sub-header/SubHeader.tsx
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
- i18n/en-US.properties
- src/elements/common/sub-header/SubHeaderRight.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
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.
📚 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/messages.tssrc/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx
📚 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/stories/tests/MetadataView-visual.stories.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 (1)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
156-156: Nice reuse via extracted propsSwitching to
metadataViewV2WithInlineCustomActionsElementPropsreduces duplication in stories.
jpan-box
force-pushed
the
mdv-bca
branch
2 times, most recently
from
5426699 to
49e0580
Compare
Switch await pattern
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx (1)
39-39: Call renderComponent() with no args when you want the defaults.Passing defaultProps again is unnecessary since renderComponent already applies them.
Apply this diff:
- renderComponent(defaultProps); + renderComponent();Also applies to: 55-55, 68-68, 82-82
🧹 Nitpick comments (5)
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx (4)
28-32: Avoid re-spreading defaultProps inside overrides.renderComponent already spreads defaultProps; re-spreading them in each call is redundant and can mask future changes by altering precedence. Pass only the overrides.
Apply these diffs:
@@ - renderComponent({ - ...defaultProps, - currentCollection: { items: [{ id: '1' }] }, - gridColumnCount: 3, - }); + renderComponent({ + currentCollection: { items: [{ id: '1' }] }, + gridColumnCount: 3, + }); @@ - renderComponent({ - ...defaultProps, - currentCollection: { items: [{ id: '1' }] }, - gridColumnCount: 3, - }); + renderComponent({ + currentCollection: { items: [{ id: '1' }] }, + gridColumnCount: 3, + }); @@ - renderComponent({ - ...defaultProps, - canCreateNewFolder: true, - currentCollection: { items: [{ id: '1' }] }, - }); + renderComponent({ + canCreateNewFolder: true, + currentCollection: { items: [{ id: '1' }] }, + });Also applies to: 46-50, 73-77
99-118: Nice parameterized coverage for ‘all’ vs Set selections.This ensures the trigger renders for both selection modes under the feature flag.
For slightly more resilient queries against i18n/title-casing, consider a case-insensitive name matcher:
- expect(screen.getByRole('button', { name: 'Bulk actions' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /bulk actions/i })).toBeInTheDocument();
120-151: Verify userEvent setup style matches your test-utils wrapper.If your testing-library wrapper re-exports @testing-library/user-event v14, the recommended usage is
userEvent.setup(). IfuserEvent()is a convenience your wrapper provides, you’re fine; otherwise switch to.setup().Apply this if needed:
- const user = userEvent(); + const user = userEvent.setup();Also, add a complementary assertion to cover Set selections to ensure onClick receives the exact Set instance:
@@ test('should call onClick when a bulk item action is clicked', async () => { @@ expect(mockOnClick).toHaveBeenCalledWith(expectedOnClickArgument); }); + + test('should pass a Set of selected IDs to onClick when selection is a Set', async () => { + const mockOnClick = jest.fn(); + const user = userEvent.setup(); + const features = { contentExplorer: { metadataViewV2: true } }; + const selection = new Set(['1', '2']); + + renderComponent( + { + ...metadataViewV2Props, + selectedItemIds: selection, + bulkItemActions: [{ label: 'Download', onClick: mockOnClick }], + }, + features, + ); + + await user.click(await screen.findByRole('button', { name: /bulk actions/i })); + await user.click(screen.getByRole('menuitem', { name: 'Download' })); + expect(mockOnClick).toHaveBeenCalledWith(selection); + });
152-164: Good negative test for feature-flag gating.Confirms both Bulk actions and Metadata controls are withheld when the flag is off.
Consider an additional negative test where
bulkItemActionsis an empty array (or undefined) but the flag is on, to lock in the “show trigger only when actions exist” behavior.src/elements/common/sub-header/SubHeaderRight.tsx (1)
73-73: Guard Selection union properly; avoid accessing.sizeon non-Set.Selection is
Set<Key> | 'all' | undefined. Accessing.sizewithout a Set check can trip TypeScript and is less explicit. Use aninstanceof Setguard (or a type predicate) for clarity and type-safety.Apply this diff:
- const hasSelectedItems: boolean = !!(selectedItemIds && (selectedItemIds === 'all' || selectedItemIds.size > 0)); + const hasSelectedItems = selectedItemIds === 'all' || (selectedItemIds instanceof Set && selectedItemIds.size > 0);Optional: if you want TS to narrow
selectedItemIdstoSelectionin JSX, introduce a type guard:const hasSelection = (s?: Selection): s is Selection => s === 'all' || (s instanceof Set && s.size > 0);Then use
isMetadataView && isMetadataViewV2Feature && hasSelection(selectedItemIds)in the render condition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/elements/common/sub-header/SubHeaderRight.tsx(5 hunks)src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx(3 hunks)src/elements/content-explorer/__tests__/ContentExplorer.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-explorer/tests/ContentExplorer.test.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
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.
📚 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/SubHeaderRight.tsx
📚 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/common/sub-header/SubHeaderRight.tsx
🧬 Code Graph Analysis (2)
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx (2)
src/elements/common/sub-header/SubHeaderRight.tsx (1)
SubHeaderRightProps(23-42)src/elements/content-explorer/ContentExplorer.tsx (1)
render(1700-1973)
src/elements/common/sub-header/SubHeaderRight.tsx (1)
src/elements/common/sub-header/BulkItemActionMenu.tsx (2)
BulkItemAction(10-13)BulkItemActionMenu(20-45)
⏰ 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 (4)
src/elements/common/sub-header/__tests__/SubHeaderRight.test.tsx (2)
24-26: LGTM: test utility now cleanly injects feature flags via wrapperProps.The render helper taking a features arg keeps tests focused and removes the need for a dedicated provider in each spec.
86-98: Good fixture setup for metadataViewV2 path.Predefining selectedItemIds, bulkItemActions, and view keeps the tests concise and aligned with the component’s contract.
src/elements/common/sub-header/SubHeaderRight.tsx (2)
23-25: Public API extension looks appropriate.Making
bulkItemActions?: BulkItemAction[]available at this level matches the wiring shown in ContentExplorer → SubHeader and keeps SubHeaderRight decoupled.
105-114: Correct: render BulkItemActionMenu only when actions exist and items are selected.This aligns with the agreed behavior that the trigger should not appear with an empty actions list.
If TypeScript complains about passing an optional
selectedItemIdstoBulkItemActionMenu(which requiresSelection), either rely on the suggested type guard in the outer condition or cast locally:- <BulkItemActionMenu actions={bulkItemActions} selectedItemIds={selectedItemIds} /> + <BulkItemActionMenu actions={bulkItemActions} selectedItemIds={selectedItemIds as Selection} />
4 participants
Assumptions:
Summary by CodeRabbit
New Features
Accessibility
Style
Tests