Skip to content

Conversation

@tjiang-box
Copy link
Contributor

@tjiang-box tjiang-box commented Oct 31, 2025

Summary by CodeRabbit

  • Chores
    • Updated several UI and metadata package versions to keep dependencies current; no changes to behavior or public APIs.
  • Tests
    • Simplified a visual test for the content sidebar redesign: reduced brittle assertions and retained selection verification to improve test robustness and maintenance.

@tjiang-box tjiang-box requested review from a team as code owners October 31, 2025 20:20
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

Walkthrough

Bumps multiple Box package versions in package.json and refactors a Storybook visual test: replaces a mutable expandButtons binding, adds nestedExpandButtons, and removes assertions that checked the "Sapporo" option's presence and aria-selected state; the test now clicks the option and asserts the selected gridcell. (42 words)

Changes

Cohort / File(s) Summary
Dependency version upgrades
package.json
Updated dependency and peerDependency versions: @box/blueprint-web^12.92.3, @box/blueprint-web-assets^4.76.5, @box/copy-input^1.5.3, @box/metadata-editor^1.5.3, @box/metadata-filter^1.41.3, @box/metadata-view^1.10.0.
Storybook visual test tweaks
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx
Replaced let expandButtons with const expandButtons, introduced const nestedExpandButtons, updated interactions to use nestedExpandButtons, and removed assertions that verified the "Sapporo" option element and its aria-selected state; test now clicks the option and asserts the resulting gridcell selection.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Review package.json peerDependency ranges for compatibility with consumers.
  • Inspect the Storybook visual test to confirm removed assertions were intentional and test still covers the intended interaction.
  • Verify the refactor from letconst and the new nestedExpandButtons do not change timing/ordering assumptions in the test.

Possibly related PRs

Suggested labels

ready-to-merge

Suggested reviewers

  • jpan-box
  • greg-in-a-box

Poem

🐇 I hopped through package pins and tests,
Swapped let for const with nimble steps,
Clicked Sapporo, found the cell,
A tidy patch — all seems well ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request lacks any description provided by the author. While the repository template appears to be guidance for the merge process rather than content requirements, best practices for dependency upgrade PRs require documentation of what was upgraded and why. The complete absence of any description makes it difficult for reviewers to understand the scope and rationale for these version bumps. Add a pull request description that documents the dependency upgrades. Include a summary of which packages were upgraded and their version ranges, any notable changes or breaking changes in those versions, and the reason for the upgrades (e.g., bug fixes, security patches, feature improvements). This will help reviewers quickly understand the impact and necessity of these changes.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title 'feat(deps): upgrade package dependencies' clearly and concisely describes the main change in the changeset. It accurately reflects that this PR is focused on upgrading package dependencies, which aligns with the raw summary showing multiple dependency version bumps in package.json. The title uses conventional commit format and is specific enough for teammates to understand the primary intent.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c92dcf5 and d7bb192.

⛔ Files ignored due to path filters (2)
  • src/elements/content-sidebar/__tests__/__snapshots__/SidebarFileProperties.test.js.snap is excluded by !**/*.snap
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (2)
  • package.json (2 hunks)
  • src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • package.json
  • src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-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

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx (2)

681-681: Verify the necessity of switching from role-based to label-based query.

Testing Library best practices recommend getByRole over getByLabelText as it better reflects how users interact with elements. Unless the expand buttons have inconsistent accessible names that make role-based queries unreliable, consider reverting to the role-based approach.

If the role-based query was causing test flakiness, the change is justified. Otherwise, consider:

-        let expandButtons = await waitFor(() => canvas.getAllByLabelText('Expand branch'));
+        let expandButtons = await waitFor(() => canvas.getAllByRole('button', { name: 'Expand branch' }));

688-688: Context change from canvas to screen may indicate portal rendering.

The switch from canvas to screen context after expanding the tree suggests the newly revealed expand buttons may render in a portal outside the canvas element. This is acceptable if accurate, but the query method change from role-based to label-based still applies (see previous comment).

If reverting to role-based queries:

-        expandButtons = await waitFor(() => screen.getAllByLabelText('Expand branch'));
+        expandButtons = await waitFor(() => screen.getAllByRole('button', { name: 'Expand branch' }));
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 87b9409 and 229b027.

⛔ Files ignored due to path filters (2)
  • src/elements/content-sidebar/__tests__/__snapshots__/SidebarFileProperties.test.js.snap is excluded by !**/*.snap
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (2)
  • package.json (2 hunks)
  • src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • package.json
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 4224
File: package.json:296-297
Timestamp: 2025-08-12T18:04:17.698Z
Learning: In the box-ui-elements project, the team is comfortable with raising peerDependency minimum versions when upgrading blueprint-web packages, even if it's a breaking change for consumers.
📚 Learning: 2025-09-03T18:10:42.760Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarNav.js:195-206
Timestamp: 2025-09-03T18:10:42.760Z
Learning: In src/elements/content-sidebar/SidebarNav.js, the maintainer wants users to be able to override data-* attributes (data-resin-target, data-target-id, data-testid) through navButtonProps for custom tabs. The spread of {...navButtonProps} should come after the data-* attributes to allow overriding, not before them.

Applied to files:

  • src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx
📚 Learning: 2025-08-25T16:19:22.007Z
Learnt from: greg-in-a-box
Repo: box/box-ui-elements PR: 4235
File: src/elements/content-explorer/MetadataQueryBuilder.ts:103-110
Timestamp: 2025-08-25T16:19:22.007Z
Learning: In src/elements/content-explorer/MetadataQueryBuilder.ts, the getSelectFilter function has a special case that maps 'mimetype-filter' to 'item.extension' because mimetype-filter is a multi-select field but the actual database field is 'item.extension'. This mapping is necessary and should not be removed.

Applied to files:

  • src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx
📚 Learning: 2025-08-28T15:38:35.122Z
Learnt from: greg-in-a-box
Repo: box/box-ui-elements PR: 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-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx
📚 Learning: 2025-08-27T17:03:48.322Z
Learnt from: bxie-box
Repo: box/box-ui-elements PR: 4250
File: src/features/classification/applied-by-ai-classification-reason/__tests__/AppliedByAiClassificationReason.test.tsx:44-51
Timestamp: 2025-08-27T17:03:48.322Z
Learning: In test files for bxie-box, prefer using queryByTestId over getByTestId when testing for expected elements that should always exist, as null access serves as validation for regressions or unexpected changes rather than masking issues with defensive assertions.

Applied to files:

  • src/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx
⏰ 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

tjuanitas
tjuanitas previously approved these changes Oct 31, 2025
reneshen0328
reneshen0328 previously approved these changes Oct 31, 2025
@mergify mergify bot added the queued label Nov 3, 2025
@mergify mergify bot merged commit 2f025e3 into box:master Nov 3, 2025
8 checks passed
@mergify mergify bot removed the queued label Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants