feat(react-tether): Upgrade react-tether to 3.0.3 #4278
Conversation
WalkthroughThis PR upgrades Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/context-menu/ContextMenu.tsx (1)
64-67: Fix disabled-toggle logic — it closes on enable, fool!Condition is reversed. If it becomes disabled while open, close it.
- // if the menu becomes disabled while it is open, we should close it - if (!isDisabled && prevIsDisabled && isOpen) { + // if the menu becomes disabled while it is open, we should close it + if (isDisabled && !prevIsDisabled && isOpen) { this.handleMenuClose(); }
🧹 Nitpick comments (13)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
201-238: Tether migration looks solid—no jive!
- Correct use of renderTarget/renderElement with refs attached.
- Conditional rendering for menu vs. confirmation is clean.
- Constraints/attachments preserved.
Optional: if any clipping occurs in complex scroll containers, consider renderElementTo="body" per react-tether v3 docs. (libraries.io)
<TetherComponent {...tetherProps} + /* Mounting to body can reduce clipping in edge cases */ + renderElementTo="body" renderTarget={ref => ( ... )} renderElement={ref => ( ... )} />src/components/context-menu/ContextMenu.tsx (2)
161-168: Gate Tether with isOpen to avoid extra workEnable Tether only when needed. Save them cycles, sucka!
const tetherProps = { attachment: 'top left', classPrefix: 'context-menu', + enabled: isOpen, constraints, targetAttachment: 'top left', targetOffset, };
171-184: Guard renderElementTo for SSR/envs without documentDirect document.body access can blow up in SSR. Add a safe guard or pass it only in the browser.
Example:
- <TetherComponent + <TetherComponent {...tetherProps} - renderElementTo={document.body} + renderElementTo={typeof document !== 'undefined' ? document.body : undefined}src/features/left-sidebar/LeftSidebarLinkCallout.js (1)
73-84: Migration LGTM — clean split of target/elementLooks tight. One nit: if you see clipping/z-index issues in scroll containers, consider portaling.
Option:
- <TetherComponent + <TetherComponent attachment={attachmentPosition} classPrefix="nav-link-callout" enabled={showTooltip} targetAttachment={targetAttachmentPosition} + renderElementTo={typeof document !== 'undefined' ? document.body : undefined}src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (2)
57-59: Ref param types: use React.Ref (callback) not RefObjectreact-tether hands you a ref callback. Don’t lie to TypeScript, fool.
- renderTarget={(ref: React.RefObject<HTMLDivElement>) => ( + renderTarget={(ref: React.Ref<HTMLSpanElement>) => ( <span ref={ref}> ... - renderElement={(ref: React.RefObject<HTMLElement>) => { + renderElement={(ref: React.Ref<HTMLSpanElement>) => { return isConfirmingDelete ? ( <span ref={ref}>Also applies to: 116-129
102-111: Localize aria-label instead of hardcoding defaultMessageMake screen readers speak the right language, sucka.
- <MenuItem - aria-label={messages.commentDeleteMenuItem.defaultMessage} + <MenuItem + aria-label={intl.formatMessage(messages.commentDeleteMenuItem)} data-resin-target={ACTIVITY_TARGETS.COMMENT_OPTIONS_DELETE}Add import and hook:
// add at top of file import { useIntl } from 'react-intl'; // inside BaseCommentMenu component before return const intl = useIntl();src/components/flyout/Flyout.js (1)
491-503: Migration LGTM — nice renderTarget/renderElement splitLooks good. Verify no clipping/z-index regressions; portal if needed.
Option:
- <TetherComponent + <TetherComponent {...tetherProps} + renderElementTo={typeof document !== 'undefined' ? document.body : undefined}src/elements/content-sidebar/activity-feed/comment/Comment.js (1)
188-258: Migration LGTM — menu/confirmation separation is cleanSolid refactor. Sanity-check overlay layering in scrollable feeds; portal if you see clipping.
Option:
- <TetherComponent + <TetherComponent attachment="top right" className="bcs-Comment-deleteConfirmationModal" constraints={[{ to: 'scrollParent', attachment: 'together' }]} targetAttachment="bottom right" + renderElementTo={typeof document !== 'undefined' ? document.body : undefined}src/components/radar/RadarAnimation.tsx (2)
144-145: Guard against SSR crash from document.bodyDirectly touching document will explode on the server, sucka. Make it optional and only set when available.
Apply this diff:
} = { attachment, classPrefix: 'radar-animation', constraints, enabled: isShown, targetAttachment, - renderElementTo: document.body, + ...(typeof document !== 'undefined' ? { renderElementTo: document.body as HTMLElement } : {}), };
160-174: Simplify target ref: prefer firstElementChildDon’t query deep descendants; anchor to the first element child. Cleaner and less surprising, fool.
Apply this diff:
- <span - ref={node => { - if (!node) { - ref.current = null; - return; - } - const firstChild = - (node.querySelector('*') as HTMLElement | null) || - (node.firstElementChild as HTMLElement | null); - ref.current = firstChild; - }} - > + <span + ref={node => { + ref.current = (node?.firstElementChild as HTMLElement | null) ?? null; + }} + >src/components/tooltip/Tooltip.tsx (3)
315-329: Tie Tether enabled to visibilityKeeping Tether always enabled wastes listeners/reflows when hidden. Hook it to showTooltip, sucka.
Apply this diff:
- renderElementTo: bodyEl, - enabled: true, + renderElementTo: bodyEl, + enabled: showTooltip,
387-412: Remove duplicate renderElementTo propYou pass renderElementTo via spread and again explicitly. Redundant. Don’t confuse future fools.
Apply this diff:
- renderElementTo={bodyEl ?? (document.body as HTMLElement)}
394-405: Use firstElementChild for target refBe explicit; avoid deep querySelector that might grab the wrong node, chump.
Apply this diff:
- <span - ref={node => { - if (!node) { - ref.current = null; - return; - } - const first = - (node.querySelector('*') as HTMLElement | null) || - (node.firstElementChild as HTMLElement | null); - ref.current = first; - }} - {...componentProps} - > + <span + ref={node => { + ref.current = (node?.firstElementChild as HTMLElement | null) ?? null; + }} + {...componentProps} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
package.json(2 hunks)src/components/context-menu/ContextMenu.tsx(1 hunks)src/components/dropdown-menu/DropdownMenu.js(1 hunks)src/components/flyout/Flyout.js(1 hunks)src/components/radar/RadarAnimation.tsx(4 hunks)src/components/tooltip/Tooltip.tsx(4 hunks)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js(1 hunks)src/elements/content-sidebar/activity-feed/app-activity/AppActivity.js(1 hunks)src/elements/content-sidebar/activity-feed/comment/Comment.js(1 hunks)src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx(1 hunks)src/elements/content-sidebar/activity-feed/task-new/Task.js(1 hunks)src/features/left-sidebar/LeftSidebarLinkCallout.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
src/components/context-menu/ContextMenu.tsx (2)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/elements/content-sidebar/activity-feed/app-activity/AppActivity.js (1)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
isConfirmingDelete(63-63)
src/components/flyout/Flyout.js (2)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)src/elements/content-sharing/ContentSharing.js (1)
isVisible(77-77)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (3)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/common/interactionTargets.js (2)
ACTIVITY_TARGETS(34-59)ACTIVITY_TARGETS(34-59)src/elements/content-sidebar/activity-feed/annotations/messages.js (1)
messages(3-50)
src/elements/content-sidebar/activity-feed/task-new/Task.js (2)
src/elements/common/interactionTargets.js (2)
ACTIVITY_TARGETS(34-59)ACTIVITY_TARGETS(34-59)src/elements/content-sidebar/activity-feed/task-new/messages.js (1)
messages(9-188)
src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (3)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (6)
handleMenuClose(99-99)isConfirmingDelete(63-63)isResolved(84-84)canDelete(79-79)handleDeleteCancel(87-87)handleDeleteConfirm(88-91)src/elements/content-sidebar/activity-feed/comment/Comment.js (4)
canResolve(164-164)isResolved(169-169)canEdit(163-163)canDelete(162-162)src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenuWrapper.js (3)
handleStatusUpdate(69-73)handleEditClick(56-60)handleDeleteClick(51-54)
src/components/radar/RadarAnimation.tsx (2)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/components/tooltip/Tooltip.tsx (2)
src/components/flyout/Flyout.js (2)
tetherProps(454-463)tetherPosition(392-392)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/elements/content-sidebar/activity-feed/comment/Comment.js (2)
src/elements/content-sidebar/activity-feed/comment/BaseComment.js (4)
canResolve(143-143)isResolved(147-147)canEdit(142-142)canDelete(141-141)src/elements/content-sidebar/activity-feed/comment/messages.js (1)
messages(9-65)
🪛 Biome (2.1.2)
src/components/dropdown-menu/DropdownMenu.js
[error] 273-273: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 276-276: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (8)
package.json (2)
344-345: Peer dep aligned; nice work, sucka!Peers reflect ^3.0.3 too. Good to prevent duplicate installs in host apps.
266-266: react-tether v3.0.3 locked down; lockfile clean, fool! Only [email protected] is hoisted—no dupes. Now pity the fool who skips updating the CI cache!src/elements/content-sidebar/activity-feed/app-activity/AppActivity.js (1)
145-171: Clean renderTarget/renderElement swap—approved, sucka!Refs are wired, conditional element rendering returns correctly, and attachments match prior behavior.
src/components/dropdown-menu/DropdownMenu.js (1)
265-273: Consider using renderElementTo instead of bodyElement for v3 consistency, fool.react-tether v3 documents renderElementTo (selector string) as the mount target. If you need custom containers, you may translate a provided element to a selector (e.g., by id) or continue relying on Tether’s bodyElement option if you’ve confirmed it still works. Validate this across Storybook/e2e to avoid clipping/regression. (libraries.io)
Would you like me to draft a backward‑compatible change that prefers renderElementTo when a stable selector is available, and falls back to current behavior otherwise?
src/components/context-menu/ContextMenu.tsx (1)
171-184: Migration LGTMrenderTarget/renderElement switch looks solid; refs and cloning are correct. I pity the fool who breaks context menus!
src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (1)
52-70: Telemetry targets: confirm EDIT vs RESOLVE for resolve/unresolveI pity the fool who mis-tags telemetry — both resolve and unresolve currently use COMMENT_OPTIONS_EDIT as the data-resin target; confirm analytics intent and change to a resolve-specific target if appropriate.
Location: src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (≈ lines 52–70)
src/components/radar/RadarAnimation.tsx (1)
21-58: Positions mapping migration looks solidEnums to TetherPosition constants are consistent. Nice move, chump.
Please sanity-check all call sites for expected visual alignment after the upgrade across common positions (TOP_* / BOTTOM_* / MIDDLE_*).
src/components/tooltip/Tooltip.tsx (1)
5-6: Type import addition is goodTyped TetherProps import aligns with the new pattern. Clean work, fool.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js (1)
43-56: Test lacks an assertion and passes a non-callable mockThat ain’t no function you passing to onClose. Use a spy and assert it was called, fool.
Apply this diff:
test('should call onClose when clicked', () => { - const callout = { - content: <div>Hi</div>, - onClose: sandbox.mock(), - }; + const callout = { + content: <div>Hi</div>, + onClose: sandbox.spy(), + }; const wrapper = mount( <LeftSidebarLinkCallout callout={callout} isShown={true}> <span>Test Child</span> </LeftSidebarLinkCallout>, ); const tetherComponent = findTetherComponent(wrapper); const btn = tetherComponent.find('.nav-link-callout-close-button').first(); btn.simulate('click'); + expect(callout.onClose.calledOnce).toBe(true); + wrapper.unmount(); });src/elements/content-sidebar/activity-feed/task-new/__tests__/Task.test.js (1)
317-332: Stop duckin’ edits, chump — fix and unskip the edit-permission test.Same issue as above; shallow won’t render portal content. Use mount + data-testid selectors to decouple from implementation.
Apply this diff:
- test.skip('should not allow user to edit if the permissions do not allow it', () => { - const wrapper = shallow( + test('should not allow user to edit if the permissions do not allow it', () => { + const anchor = document.createElement('div'); + document.body.appendChild(anchor); + const wrapper = mount( <Task {...task} permissions={{ can_delete: true, can_update: false }} currentUser={otherUser} approverSelectorContacts={approverSelectorContacts} handlers={allHandlers} onEdit={jest.fn()} - />, - ); - wrapper.find('MediaMenu[data-testid="task-actions-menu"]').simulate('click'); - wrapper.update(); - expect(wrapper.find('MenuItem[data-testid="edit-task"]')).toHaveLength(0); - expect(wrapper.find('MenuItem[data-testid="delete-task"]')).toHaveLength(1); + />, + { attachTo: anchor }, + ); + wrapper.find('[data-testid="task-actions-menu"]').hostNodes().simulate('click'); + wrapper.update(); + expect(wrapper.find('[data-testid="edit-task"]').hostNodes()).toHaveLength(0); + expect(wrapper.find('[data-testid="delete-task"]').hostNodes()).toHaveLength(1); + wrapper.unmount(); + anchor.remove(); });src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (3)
66-75: Don’t skip core behavior—mock react-tether and re‑enableThese parameterized cases validate menu visibility and messaging. Skipping them hides regressions from the upgrade.
-test.skip.each` +test.each`Add this lightweight mock at the top of the file (outside the changed lines) so portals don’t multiply nodes during tests:
jest.mock('react-tether', () => ({ __esModule: true, default: ({ renderTarget, renderElement }) => ( <> {typeof renderTarget === 'function' ? renderTarget() : null} {typeof renderElement === 'function' ? renderElement() : null} </> ), }));Re-enable the truth, fool!
101-119: Re-enable edit flow; drop dive() with mountUse the menu’s onEdit handler and force an update; dive() is for shallow wrappers.
-test.skip('should render CommentForm if user clicks on the Modify menu item', () => { +test('should render CommentForm if user clicks on the Modify menu item', () => { @@ - const wrapper = getWrapper({ ...mockActivity, ...activity }); - - wrapper.find(AnnotationActivityMenu).dive().simulate('click'); - wrapper.find(AnnotationActivityMenu).dive().find('MenuItem').simulate('click'); + const wrapper = getWrapper({ ...mockActivity, ...activity }); + // trigger edit from menu + wrapper.find(AnnotationActivityMenu).prop('onEdit')(); + wrapper.update(); expect(wrapper.exists('ForwardRef(withFeatureConsumer(CommentForm))')).toBe(true); // Firing the onCancel prop will remove the CommentForm - wrapper.find('ForwardRef(withFeatureConsumer(CommentForm))').props().onCancel(); + wrapper.find('ForwardRef(withFeatureConsumer(CommentForm))').props().onCancel(); + wrapper.update(); expect(wrapper.exists('ForwardRef(withFeatureConsumer(CommentForm))')).toBe(false); });Quit diving when you already mounted, fool!
227-271: Unskip and fix delete-confirmation assertions; add updatesThis suite exercises critical UX. Also, the first test’s expect lacks an assertion.
-describe.skip('delete confirmation behavior', () => { +describe('delete confirmation behavior', () => { @@ - wrapper.find(AnnotationActivityMenu).prop('onDelete')(); - - expect(wrapper.exists(DeleteConfirmation)); + wrapper.find(AnnotationActivityMenu).prop('onDelete')(); + wrapper.update(); + expect(wrapper.exists(DeleteConfirmation)).toBe(true); @@ - wrapper.find(AnnotationActivityMenu).prop('onDelete')(); - wrapper.find(DeleteConfirmation).prop('onDeleteCancel')(); + wrapper.find(AnnotationActivityMenu).prop('onDelete')(); + wrapper.update(); + wrapper.find(DeleteConfirmation).prop('onDeleteCancel')(); + wrapper.update(); @@ - wrapper.find(AnnotationActivityMenu).prop('onDelete')(); - wrapper.find(DeleteConfirmation).prop('onDeleteConfirm')(); + wrapper.find(AnnotationActivityMenu).prop('onDelete')(); + wrapper.update(); + wrapper.find(DeleteConfirmation).prop('onDeleteConfirm')(); + wrapper.update();Ain’t no skipping when confirming delete, fool!
🧹 Nitpick comments (37)
src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js (1)
66-83: Tighten selector assertions and clean up after mountCheck the node you found, assert via hasClass, and unmount to avoid leaks. Don’t leave junk around, fool.
Apply this diff:
- const wrapper = mount( + const wrapper = mount( <LeftSidebarLinkCallout callout={{ content: <div>Hi</div>, onClose: () => {}, }} isShown={true} navLinkClassName="testClass" > <span>Test Child</span> </LeftSidebarLinkCallout>, ); - const tetherComponent = findTetherComponent(wrapper); - const callout = tetherComponent.find('.nav-link-callout'); - expect(callout.props().className).toContain('testClass'); + const tetherComponent = findTetherComponent(wrapper); + const calloutNode = tetherComponent.find('.nav-link-callout').first(); + expect(calloutNode.exists()).toBe(true); + expect(calloutNode.hasClass('testClass')).toBe(true); + wrapper.unmount();src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (7)
2-2: Mount adds DOM side effects—clean it up, fool!Switching to mount increases side effects. Add wrapper cleanup and attach to a stable container to avoid leaking portals and flaky assertions.
Apply this diff to stabilize mounting:
-import { mount } from 'enzyme'; +import { mount } from 'enzyme'; +import { act } from 'react-dom/test-utils';Add this helper outside the changed lines to ensure cleanup:
// Test utils for this file const mountedWrappers = []; afterEach(() => { mountedWrappers.splice(0).forEach(w => { try { w.unmount(); } catch (_) {} }); });I pity the fool who forgets to unmount!
49-49: Attach mount to a real container to tame portal duplicatesMount the component into document.body so tethered portals render predictably under react-tether 3.x.
-const getWrapper = (props = {}) => mount(<AnnotationActivity {...mockActivity} {...props} />); +const getWrapper = (props = {}) => { + const wrapper = mount(<AnnotationActivity {...mockActivity} {...props} />, { attachTo: document.body }); + // track for cleanup + mountedWrappers.push(wrapper); + return wrapper; +};Do it right, fool!
124-127: Target the right link; ditch brittle first()Select by message id to avoid accidental matches from duplicated tether nodes.
- expect(wrapper.find('AnnotationActivityLink').first().prop('message')).toEqual({ + const activityLink = wrapper + .find('AnnotationActivityLink') + .filterWhere(n => (n.prop('message') || {}).id === messages.annotationActivityVersionLink.id) + .first(); + expect(activityLink.prop('message')).toEqual({ ...messages.annotationActivityVersionLink, values: { number: '2' }, });Precision over luck, fool!
132-132: Same here—filter by descriptor, not positionAvoid first(); match the version-unavailable message explicitly.
- const activityLink = wrapper.find('AnnotationActivityLink').first(); + const activityLink = wrapper + .find('AnnotationActivityLink') + .filterWhere(n => (n.prop('message') || {}).id === messages.annotationActivityVersionUnavailable.id) + .first();Don’t gamble with first(), fool!
314-322: Overflow menu case should run—trigger and updateRe-enable and update after invoking onMenuOpen.
-test.skip('should disable card if the overflow menu is open', () => { +test('should disable card if the overflow menu is open', () => { @@ - wrapper.find(AnnotationActivityMenu).prop('onMenuOpen')(); + wrapper.find(AnnotationActivityMenu).prop('onMenuOpen')(); + wrapper.update();Open the menu the right way, fool!
324-332: Editing case should run—use onEdit + updateRe-enable to cover the isEditing path.
-test.skip('should disable card if editing the comment', () => { +test('should disable card if editing the comment', () => { @@ - wrapper.find(AnnotationActivityMenu).prop('onEdit')(); + wrapper.find(AnnotationActivityMenu).prop('onEdit')(); + wrapper.update();Test that edit flow, fool!
340-348: Delete-confirmation open path should runRe-enable and update after onDelete.
-test.skip('should disable card if the delete confirmation is open', () => { +test('should disable card if the delete confirmation is open', () => { @@ - wrapper.find(AnnotationActivityMenu).prop('onDelete')(); + wrapper.find(AnnotationActivityMenu).prop('onDelete')(); + wrapper.update();Ain’t no fear of confirm, fool—test it!
src/components/radar/__tests__/RadarAnimation.test.tsx (1)
14-16: Make tether lookup precise.findWhere(renderTarget && renderElement) can match unintended nodes. Narrow by component displayName or a stable prop (e.g., classPrefix === 'radar') to avoid false positives.
-const findTetherComponent = (wrapper: ReactWrapper) => { - return wrapper.findWhere(node => node.prop('renderTarget') && node.prop('renderElement')); -}; +const findTetherComponent = (wrapper: ReactWrapper) => + wrapper.findWhere( + node => + node.prop('renderTarget') && + node.prop('renderElement') && + node.prop('classPrefix') === 'radar', + );src/components/flyout/__tests__/Flyout.test.js (2)
48-50: Tighten Tether match, fool.Same nit as elsewhere: guard on classPrefix === 'flyout-overlay' to avoid matching unrelated components.
-const findTetherComponent = wrapper => { - return wrapper.findWhere(node => node.prop('renderTarget') && node.prop('renderElement')); -}; +const findTetherComponent = wrapper => + wrapper.findWhere( + node => + node.prop('renderTarget') && + node.prop('renderElement') && + node.prop('classPrefix') === 'flyout-overlay', + );
234-245: Fix test name to match behavior.Title says constrainToScrollParent=true, but the test sets constrainToWindow=true. Rename it, fool.
-test('should render TetherComponent with window constraint when constrainToScrollParent=true', () => { +test('should render TetherComponent with window constraint when constrainToWindow=true', () => {src/components/dropdown-menu/__tests__/DropdownMenu.test.js (1)
33-35: Harden Tether selector.Gate on classPrefix === 'dropdown-menu' so your findWhere doesn’t snag the wrong node.
-const findTetherComponent = wrapper => { - return wrapper.findWhere(node => node.prop('renderTarget') && node.prop('renderElement')); -}; +const findTetherComponent = wrapper => + wrapper.findWhere( + node => + node.prop('renderTarget') && + node.prop('renderElement') && + node.prop('classPrefix') === 'dropdown-menu', + );src/components/context-menu/__tests__/ContextMenu.test.tsx (1)
252-254: Minimize redundant state set, fool.Setting isOpen to true when already true is fine for coverage, but consider asserting idempotence without extra setState churn.
src/components/tooltip/Tooltip.tsx (3)
315-327: Duplicate renderElementTo prop—pick one.You pass renderElementTo via tetherProps and again on the JSX. Drop the duplicate to avoid confusion.
targetAttachment: tetherPosition.targetAttachment, - renderElementTo: bodyEl, }; ... <TetherComponent {...tetherProps} ref={this.tetherRef} - renderElementTo={bodyEl ?? (document.body as HTMLElement)} renderTarget={(ref: React.MutableRefObject<HTMLElement>) => {Also applies to: 386-411
391-408: Target ref resolution could pick a nested descendant.querySelector('*') grabs the first nested element, not necessarily the direct child. If that’s intentional, keep it; otherwise prefer firstElementChild for determinism.
-const first = - (node.querySelector('*') as HTMLElement | null) || - (node.firstElementChild as HTMLElement | null); +const first = node.firstElementChild as HTMLElement | null;
215-221: Keydown handler fires child onKeyDown with a native event.Most children expect a React.KeyboardEvent. If any consumers rely on SyntheticEvent shape, consider wrapping or guarding.
src/components/text-input/__tests__/TextInput.test.js (1)
83-85: Tether locator is too loose; tighten and assert count, fool!Add an explicit length assertion to avoid false positives if multiple nodes match, and consider unmounting to prevent leakage across tests.
- const tetherEl = wrapper.findWhere(node => node.prop('renderTarget') && node.prop('renderElement')); + const tetherEl = wrapper.findWhere( + node => typeof node.prop('renderTarget') === 'function' && typeof node.prop('renderElement') === 'function', + ); + expect(tetherEl).toHaveLength(1); + wrapper.unmount();src/components/text-area/__tests__/TextArea.test.js (1)
75-77: Lock down the tether selector and clean up after yourself, sucker!Same fragility as TextInput: assert exactly one tether match and unmount to avoid side effects.
- const tetherEl = wrapper.findWhere(node => node.prop('renderTarget') && node.prop('renderElement')); + const tetherEl = wrapper.findWhere( + node => typeof node.prop('renderTarget') === 'function' && typeof node.prop('renderElement') === 'function', + ); + expect(tetherEl).toHaveLength(1); + wrapper.unmount();src/components/tooltip/__tests__/Tooltip.test.tsx (19)
72-88: Default render test: assert one tether and unmount, fool!Strengthen by checking exactly one tether node and cleaning up.
- const tetherComponent = wrapper.findWhere(node => node.prop('renderTarget') && node.prop('renderElement')); + const tetherComponent = wrapper.findWhere( + node => typeof node.prop('renderTarget') === 'function' && typeof node.prop('renderElement') === 'function', + ); + expect(tetherComponent).toHaveLength(1); @@ - expect(tetherComponent.prop('targetAttachment')).toEqual('top center'); + expect(tetherComponent.prop('targetAttachment')).toEqual('top center'); + wrapper.unmount();
98-106: isTabbable=false test is solid.Consider unmount for hygiene.
- const component = wrapper.find('button'); + const component = wrapper.find('button'); expect(component.prop('tabIndex')).toBeFalsy(); + wrapper.unmount();
109-123: Visibility via mouseenter: add cleanup, fool!Mount + simulate looks good; add unmount.
expect(tooltip.text()).toEqual('hi'); + wrapper.unmount();
183-194: bodyElement wiring: assert one tether and unmount, fool!Tighten selector/count and clean up.
- expect( - wrapper - .findWhere(node => node.prop('renderTarget') && node.prop('renderElement')) - .prop('renderElementTo'), - ).toEqual(bodyEl); + const tether = wrapper.findWhere( + n => typeof n.prop('renderTarget') === 'function' && typeof n.prop('renderElement') === 'function', + ); + expect(tether).toHaveLength(1); + expect(tether.prop('renderElementTo')).toEqual(bodyEl); + wrapper.unmount();
197-209: Invalid body element fallback: same tightening applies.Ensure single match and unmount.
- expect( - wrapper - .findWhere(node => node.prop('renderTarget') && node.prop('renderElement')) - .prop('renderElementTo'), - ).toEqual(document.body); + const tether = wrapper.findWhere( + n => typeof n.prop('renderTarget') === 'function' && typeof n.prop('renderElement') === 'function', + ); + expect(tether).toHaveLength(1); + expect(tether.prop('renderElementTo')).toEqual(document.body); + wrapper.unmount();
212-231: isShown=true path: add cleanup.Assertions look right; add unmount.
expect(tooltip.text()).toEqual('hi'); + wrapper.unmount();
234-244: aria-describedby when aria-label differs: add cleanup.expect(component.prop('aria-describedby')).toEqual(tooltip.prop('id')); + wrapper.unmount();
246-254: No aria-describedby when aria-label equals text: add cleanup.expect(component.prop('aria-describedby')).toEqual(undefined); + wrapper.unmount();
257-265: aria-hidden true when label equals text: add cleanup.expect(tooltip.prop('aria-hidden')).toBe(true); + wrapper.unmount();
267-275: ariaHidden prop forces aria-hidden: add cleanup.expect(tooltip.prop('aria-hidden')).toBe(true); + wrapper.unmount();
277-285: ariaHidden suppresses aria-describedby: add cleanup.expect(component.prop('aria-describedby')).toBe(undefined); + wrapper.unmount();
287-295: ariaHidden suppresses aria-errormessage: add cleanup.expect(component.prop('aria-errormessage')).toBe(undefined); + wrapper.unmount();
298-306: aria-hidden false without aria-label: add cleanup.expect(tooltip.prop('aria-hidden')).toBe(false); + wrapper.unmount();
309-317: aria-hidden false when label differs: add cleanup.expect(tooltip.prop('aria-hidden')).toBe(false); + wrapper.unmount();
321-331: Error theme: assertions good; add cleanup.expect(component.prop('aria-errormessage')).toEqual(tooltip.prop('id')); + wrapper.unmount();
350-356: Snapshot of stopBubble: add cleanup, fool!expect(wrapper.find('div.tooltip')).toMatchSnapshot(); + wrapper.unmount();
359-365: Event capture absent when stopBubble not set: add cleanup.expect(wrapper.find('div[role="presentation"]').exists()).toBe(false); + wrapper.unmount();
530-539: This "keydown disabled" test doesn’t test keydown — exercise Escape and assert no hide, fool!Currently it only asserts mouseenter shows. Simulate Escape and verify isShown stays true.
- // Trigger mouseenter to show tooltip - wrapper.find('button').simulate('mouseenter'); - expect(wrapper.state('isShown')).toBe(true); + // Show tooltip first + wrapper.find('button').simulate('mouseenter'); + expect(wrapper.state('isShown')).toBe(true); + // Keydown should not hide when functionality is disabled + wrapper.find('button').simulate('keydown', { key: 'Escape' }); + expect(wrapper.state('isShown')).toBe(true); + wrapper.unmount();
558-563: position() test: mount is fine, but shallow would also work; add cleanup.Optional: use shallow since you stub tetherRef anyway.
- const wrapper = mount( + const wrapper = mount( <Tooltip text="hi" isShown={isShown}> <button /> </Tooltip>, ); @@ expect(positionTetherMock).toHaveBeenCalledTimes(isShown ? 1 : 0); + wrapper.unmount();Alternative:
- const wrapper = mount( + const wrapper = shallow<Tooltip>( <Tooltip text="hi" isShown={isShown}> <button /> </Tooltip>, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
src/components/media/__tests__/__snapshots__/Media.test.tsx.snapis excluded by!**/*.snapsrc/components/radar/__tests__/__snapshots__/RadarAnimation.test.tsx.snapis excluded by!**/*.snapsrc/components/sidebar-toggle-button/__tests__/__snapshots__/SidebarToggleButton.test.js.snapis excluded by!**/*.snapsrc/components/tooltip/__tests__/__snapshots__/Tooltip.test.tsx.snapis excluded by!**/*.snapsrc/elements/content-sidebar/activity-feed/app-activity/__tests__/__snapshots__/AppActivity.test.js.snapis excluded by!**/*.snapsrc/elements/content-sidebar/activity-feed/comment/__tests__/__snapshots__/Comment.test.js.snapis excluded by!**/*.snapsrc/elements/content-sidebar/activity-feed/task-new/__tests__/__snapshots__/Task.test.js.snapis excluded by!**/*.snapsrc/features/left-sidebar/__tests__/__snapshots__/LeftSidebarLinkCallout.test.js.snapis excluded by!**/*.snapsrc/features/pagination/__tests__/__snapshots__/MarkerBasedPagination.test.js.snapis excluded by!**/*.snapsrc/features/pagination/__tests__/__snapshots__/OffsetBasedPagination.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
src/components/context-menu/__tests__/ContextMenu.test.tsx(9 hunks)src/components/dropdown-menu/__tests__/DropdownMenu.test.js(17 hunks)src/components/flyout/__tests__/Flyout.test.js(20 hunks)src/components/pill-selector-dropdown/__tests__/PillSelector.test.js(16 hunks)src/components/radar/__tests__/RadarAnimation.test.tsx(2 hunks)src/components/text-area/__tests__/TextArea.test.js(2 hunks)src/components/text-input/__tests__/TextInput.test.js(1 hunks)src/components/tooltip/Tooltip.tsx(3 hunks)src/components/tooltip/__tests__/Tooltip.test.tsx(14 hunks)src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js(9 hunks)src/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.js(1 hunks)src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js(3 hunks)src/elements/content-sidebar/activity-feed/task-new/__tests__/Task.test.js(2 hunks)src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-15T17:04:28.279Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4275
File: src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js:259-260
Timestamp: 2025-09-15T17:04:28.279Z
Learning: When wrapping enzyme mount() tests with IntlProvider for class-based components, use wrapper.find('ComponentName') to access the actual component instance and state, since the wrapper becomes the provider instead of the component.
Applied to files:
src/components/text-input/__tests__/TextInput.test.jssrc/components/text-area/__tests__/TextArea.test.jssrc/components/context-menu/__tests__/ContextMenu.test.tsxsrc/components/radar/__tests__/RadarAnimation.test.tsxsrc/components/flyout/__tests__/Flyout.test.jssrc/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.jssrc/components/tooltip/__tests__/Tooltip.test.tsx
📚 Learning: 2025-08-19T17:50:47.187Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4228
File: src/elements/content-sidebar/activity-feed/comment/Comment.js:147-148
Timestamp: 2025-08-19T17:50:47.187Z
Learning: In the box-ui-elements codebase, ActivityThreadReplyForm.js and AnnotationThreadCreate.js component files exist but are not actively used/imported anywhere, so they don't need updates for prop propagation.
Applied to files:
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js
📚 Learning: 2025-08-27T17:03:48.322Z
Learnt from: bxie-box
PR: box/box-ui-elements#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/components/tooltip/__tests__/Tooltip.test.tsx
🧬 Code graph analysis (6)
src/components/text-input/__tests__/TextInput.test.js (1)
src/components/text-input/TextInput.js (1)
TextInput(42-100)
src/components/text-area/__tests__/TextArea.test.js (1)
src/components/text-area/TextArea.js (1)
TextArea(33-91)
src/components/context-menu/__tests__/ContextMenu.test.tsx (1)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (1)
FakeMenu(16-20)
src/components/radar/__tests__/RadarAnimation.test.tsx (1)
src/components/radar/RadarAnimation.tsx (1)
RadarAnimationProps(60-77)
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (2)
src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js (1)
getWrapper(21-21)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
AnnotationActivity(48-98)
src/components/tooltip/Tooltip.tsx (3)
src/components/flyout/Flyout.js (2)
tetherProps(454-463)tetherPosition(392-392)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)src/features/left-sidebar/LeftSidebarLinkCallout.js (1)
showTooltip(51-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (12)
src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js (2)
2-2: LGTM on adding mount for tethered UIGood call bringing in mount to exercise the tethered content, fool. I pity the fool who tries to test portals with shallow.
7-9: Make findTetherComponent tighter and fail fastGrab the first match and guard against empty results to avoid flaky tests, sucka.
[ suggest_nitpick_refactor ]
Apply this diff:-const findTetherComponent = wrapper => { - return wrapper.findWhere(node => node.prop('renderTarget') && node.prop('renderElement')); -}; +const findTetherComponent = wrapper => { + const tether = wrapper + .findWhere(node => Boolean(node.prop('renderTarget')) && Boolean(node.prop('renderElement'))) + .first(); + expect(tether.exists()).toBe(true); + return tether; +};src/elements/content-sidebar/activity-feed/task-new/__tests__/Task.test.js (1)
300-315: Re-enable portal-attached permission tests; mount to a real DOM anchor.I pity the fool who skips permission checks — re-enable these tests, mount them to a real DOM (attachTo anchor), simulate the host node click (hostNodes()), assert hostNodes() for menu items, then unmount and remove the anchor. Ensure mount is imported if not already.
Apply this diff:
- test.skip('should not allow user to delete if the task permissions do not allow it', () => { - const wrapper = shallow( + test('should not allow user to delete if the task permissions do not allow it', () => { + const anchor = document.createElement('div'); + document.body.appendChild(anchor); + const wrapper = mount( <Task {...task} permissions={{ can_delete: false, can_update: true }} currentUser={otherUser} approverSelectorContacts={approverSelectorContacts} handlers={allHandlers} onDelete={jest.fn()} - />, - ); - wrapper.find('MediaMenu[data-testid="task-actions-menu"]').simulate('click'); - wrapper.update(); - expect(wrapper.find('MenuItem[data-testid="delete-task"]')).toHaveLength(0); - expect(wrapper.find('MenuItem[data-testid="edit-task"]')).toHaveLength(1); + />, + { attachTo: anchor }, + ); + wrapper.find('[data-testid="task-actions-menu"]').hostNodes().simulate('click'); + wrapper.update(); + expect(wrapper.find('[data-testid="delete-task"]').hostNodes()).toHaveLength(0); + expect(wrapper.find('[data-testid="edit-task"]').hostNodes()).toHaveLength(1); + wrapper.unmount(); + anchor.remove(); });Quick sweep found skipped tests in:
- src/utils/tests/datetime.test.ts
- src/features/message-preview-content/tests/MessagePreviewContent.test.js
- src/elements/content-sidebar/activity-feed/task-new/tests/Task.test.js
- src/elements/content-sidebar/activity-feed/annotations/tests/AnnotationActivity.test.js
- src/elements/content-sidebar/activity-feed/app-activity/tests/AppActivity.test.js
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (1)
66-75: Quick repo check — track skipped tests now, not laterFound 17 skipped tests repo-wide; AnnotationActivity.test.js has 6 skipped tests (lines 66, 101, 227, 314, 324, 340). Remove or unskip tests, add explicit justifications/issue links for any remaining skips, or open a tracking ticket — don’t leave them undocumented. I pity the fool who ignores skipped tests.
src/components/pill-selector-dropdown/__tests__/PillSelector.test.js (1)
280-281: LGTM on event targeting.Routing click/keyDown through .bdl-PillSelector mirrors actual focus/keyboard behavior. Tight and tidy.
Also applies to: 308-313, 321-325, 334-339, 348-354, 358-363, 375-379, 389-394, 409-413, 422-427, 447-452, 466-471, 477-482, 493-494, 503-504
src/components/context-menu/__tests__/ContextMenu.test.tsx (1)
105-119: LGTM on tether prop assertions.Mount + findWhere checks the right props after the migration. Solid.
src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js (1)
107-141: Unskip the parameterized Comment.test suites (or convert to test.todo) — I pity the fool who hides regressions.
- The three parameterized suites in src/elements/content-sidebar/activity-feed/comment/tests/Comment.test.js (≈ lines 107–208) are skipped; rework them for the renderTarget/renderElement pattern or mark them as test.todo and link a follow‑up issue.
- The supplied ripgrep run returned no output; I couldn't verify. Re-run from the repo root: rg -n "test\.skip" -S --hidden -g '!node_modules' and attach the output.
src/components/text-area/__tests__/TextArea.test.js (1)
2-2: Import change LGTM.Mount is appropriate here for verifying tether props. I pity the fool who shallow-renders portals.
src/components/tooltip/__tests__/Tooltip.test.tsx (4)
5-5: Adding mount is the right move.Good switch to full DOM rendering for tether assertions. I pity the fool who tests portals with shallow.
126-133: Custom class on tooltip: LGTM.Mount-based assertion is appropriate.
389-407: StopBubble handlers: LGTM.Good verification of both stopPropagation and stopImmediatePropagation. I pity the fool who lets events leak.
544-551: Child onKeyDown piping: LGTM.Mount + simulate is correct.
src/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/elements/content-sidebar/activity-feed/task-new/Task.js (1)
329-340: I pity the fool who left that return statement out!The
renderElementcallback's using a block body with braces but ain't returning nothing! That DeleteConfirmation modal's never gonna show up, fool!Apply this fix to make it work right:
renderElement={ref => { - return isConfirmingDelete ? ( + return isConfirmingDelete ? ( <span ref={ref}> <DeleteConfirmation data-resin-component={ACTIVITY_TARGETS.TASK_OPTIONS} isOpen={isConfirmingDelete} message={messages.taskDeletePrompt} onDeleteCancel={this.handleDeleteCancel} onDeleteConfirm={this.handleDeleteConfirm} /> </span> ) : null; }}Or make it cleaner with an implicit return, fool:
- renderElement={ref => { - return isConfirmingDelete ? ( - <span ref={ref}> - <DeleteConfirmation - data-resin-component={ACTIVITY_TARGETS.TASK_OPTIONS} - isOpen={isConfirmingDelete} - message={messages.taskDeletePrompt} - onDeleteCancel={this.handleDeleteCancel} - onDeleteConfirm={this.handleDeleteConfirm} - /> - </span> - ) : null; - }} + renderElement={ref => + isConfirmingDelete ? ( + <span ref={ref}> + <DeleteConfirmation + data-resin-component={ACTIVITY_TARGETS.TASK_OPTIONS} + isOpen={isConfirmingDelete} + message={messages.taskDeletePrompt} + onDeleteCancel={this.handleDeleteCancel} + onDeleteConfirm={this.handleDeleteConfirm} + /> + </span> + ) : null + }src/components/dropdown-menu/__tests__/DropdownMenu.test.js (2)
150-161: I pity the fool — still usin' bodyElement prop (shoulda been fixed)!Previous review flagged this: react-tether v3 expects
renderElementTo, notbodyElement. The test's assertin'renderElementTocorrectly (line 156), but if the component's still passin'bodyElementprop, that's gonna fail at runtime.#!/bin/bash # I pity the fool who doesn't check if DropdownMenu.js is usin' the right prop! # Lookin' for bodyElement vs renderElementTo usage in the component echo "=== Checkin' DropdownMenu.js for bodyElement/renderElementTo usage ===" fd -e js -e jsx -e ts -e tsx "DropdownMenu" --exec grep -n "bodyElement\|renderElementTo" {} + echo -e "\n=== Showin' the Tether prop passin' in DropdownMenu.js ===" ast-grep --pattern 'TetherComponent $$$' src/components/dropdown-menu/DropdownMenu.js | head -50
171-184: Listen up, fool — same bodyElement issue here!Test expects
renderElementTo(line 179), but still passin'bodyElementto the wrapper. If the component ain't migrated yet, this test's gonna blow up like Hannibal's plan!
🧹 Nitpick comments (7)
src/features/unified-share-modal/UnifiedShareModal.scss (1)
205-207: Narrow the selector to the tooltip wrapper to avoid flex glitches, fool.
> span { width: 100% }will stretch any direct span child and can jank the 80/20 flex layout if another span slips in. Target the tooltip wrapper class instead.- > span { - width: 100%; // This fixes a width issue with Tooltip wrapper - } + // Limit to tooltip target/wrapper span to avoid affecting unrelated spans + > .bdl-targeted-click-through, + > .bdl-Tooltip-target { + width: 100%; + }src/components/button-group/ButtonGroup.scss (1)
46-67: Selector expansion looks solid. Add nested .bdl-Button in disabled block too.You handled nested
.btncases; mirror that for.bdl-Buttonwhen disabled so styles are consistent.&.bdl-is-disabled, &.is-disabled { &, & > .bdl-targeted-click-through { - > .btn, - > .bdl-Button { + > .btn, + > .bdl-Button, + > * > .btn, + > * > .bdl-Button { color: $bdl-gray-65; background-color: $bdl-gray-02; border: 1px solid $bdl-gray-30; box-shadow: none; cursor: default; opacity: 0.4; }Also applies to: 68-76, 74-78, 80-87, 89-92
src/components/checkbox/Checkbox.scss (1)
75-82: Lose the 0.1 line-height hack, chump—use 0 or align-items instead.
line-height: 0.1is brittle. Preferline-height: 0(common for icon wrappers) or rely on flex centering.-.checkbox-tooltip-wrapper { +.checkbox-tooltip-wrapper { display: inline-flex; vertical-align: text-bottom; - line-height: 0.1; // This keeps the tooltip wrapper height consistent with the child element + line-height: 0; // Collapse inline line-box reliablysrc/components/dropdown-menu/DropdownMenu.js (1)
271-291: Ref forwarding: don’t use querySelector('*'); anchor to the immediate child, fool.
querySelector('*')may grab a grandchild (e.g., an icon) and offset the tether. UsefirstElementChildand fall back safely.- renderTarget={ref => ( + renderTarget={ref => ( <span - ref={node => { - if (!node) { - ref.current = null; - return; - } - const first = node.querySelector('*') || node.firstElementChild; - ref.current = first; - }} + ref={node => { + if (!node) { + ref.current = null; + return; + } + ref.current = node.firstElementChild || null; + }} > {React.cloneElement(menuButton, menuButtonProps)} </span> )}src/components/radar/RadarAnimation.tsx (2)
156-174: Type the ref param as nullable and drop the deep selector, fool.
MutableRefObject<HTMLElement>should beHTMLElement | null. Also preferfirstElementChildto avoid grabbing nested icons.- renderTarget={(ref: React.MutableRefObject<HTMLElement>) => ( + renderTarget={(ref: React.MutableRefObject<HTMLElement | null>) => ( <span ref={node => { if (!node) { ref.current = null; return; } - const firstChild = - (node.querySelector('*') as HTMLElement | null) || - (node.firstElementChild as HTMLElement | null); - ref.current = firstChild; + ref.current = node.firstElementChild as HTMLElement | null; }} > {referenceElement} </span> )}
175-181: Element render is unconditional; double-check hidden state and a11y.With
enabledcontrolling tethering, the radar DOM still mounts. Ensure it’s aria-hidden or not rendered whenisShownis false to avoid stray tab stops.- renderElement={(ref: React.RefObject<HTMLDivElement>) => ( - <div ref={ref} className={`radar ${className}`} id={this.radarAnimationID} {...rest}> + renderElement={(ref: React.RefObject<HTMLDivElement>) => ( + isShown ? ( + <div ref={ref} className={`radar ${className}`} id={this.radarAnimationID} {...rest}> <div className="radar-dot" /> <div className="radar-circle" /> - </div> + </div> + ) : null )}src/components/dropdown-menu/__tests__/DropdownMenu.test.js (1)
297-301: Consider replacin' Sinon mocks with Jest, sucka!You got Jest for some tests but still usin' Sinon sandbox mocks. Mix and match ain't the cleanest approach — pick one mockin' library and stick with it like B.A. sticks to his van!
Example for line 297-301:
-sandbox.mock(instance).expects('setState').withArgs({ - isOpen: true, - initialFocusIndex: 1, -}); +const setStateSpy = jest.spyOn(instance, 'setState'); instance.openMenuAndSetFocusIndex(1); +expect(setStateSpy).toHaveBeenCalledWith({ + isOpen: true, + initialFocusIndex: 1, +});Also applies to: 309-311, 326-326, 381-381, 442-442
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/features/pagination/__tests__/__snapshots__/OffsetBasedPagination.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
src/components/button-group/ButtonGroup.scss(3 hunks)src/components/checkbox/Checkbox.scss(1 hunks)src/components/dropdown-menu/DropdownMenu.js(1 hunks)src/components/dropdown-menu/__tests__/DropdownMenu.test.js(17 hunks)src/components/flyout/Flyout.js(1 hunks)src/components/radar/RadarAnimation.tsx(4 hunks)src/elements/content-sidebar/activity-feed/task-new/Task.js(1 hunks)src/features/unified-share-modal/UnifiedShareModal.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/flyout/Flyout.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/elements/content-sidebar/activity-feed/task-new/Task.js (2)
src/elements/common/interactionTargets.js (2)
ACTIVITY_TARGETS(34-59)ACTIVITY_TARGETS(34-59)src/elements/content-sidebar/activity-feed/task-new/messages.js (1)
messages(9-188)
src/components/radar/RadarAnimation.tsx (2)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
⏰ 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). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (10)
src/elements/content-sidebar/activity-feed/task-new/Task.js (1)
296-341: Good job upgrading to the new TetherComponent pattern, sucka!I see you're splitting the anchor and tethered content using
renderTargetandrenderElement- that's the right move for react-tether 3.x, fool! This follows the same pattern as all them other components in this PR.src/components/dropdown-menu/DropdownMenu.js (1)
271-291: LGTM on v3 API usage (renderTarget/renderElement/renderElementTo).Clean migration, enabled gated by isOpen, no TS annotations left in JS. I like it.
Please run dropdown menu stories and keyboard/ARIA tests to confirm focus stays on the real button after wrapping.
src/components/radar/RadarAnimation.tsx (4)
23-57: Good move to TetherPosition constants, sucka.Mapping looks correct for all positions.
62-62: Children type tightened—right on.
React.ReactElementmatchesChildren.only+cloneElement.
91-97: Resolved —position()exists on react-tether v3's TetherComponent.
I pity the fool who doubts — v3 README and npm docs show the imperative position() method; no change required.
132-145: UseclassNamefor the Tether wrapper andclassPrefix/classesfor Tether’s internal classes. I pity the fool who mixes them up!src/components/dropdown-menu/__tests__/DropdownMenu.test.js (4)
24-31: Good work on the test helper, sucka!Clean helper function for mountin' — makes the tests readable and maintainable. I approve of this refactor!
33-35: Smart move with findTetherComponent, fool!This helper's doin' the heavy liftin' to find that TetherComponent by its render props. That's the A-Team way to do it!
92-94: Listen up! You're usin' act() like a champ!All state updates wrapped in act() with proper wrapper.update() calls — that's how we do it in the A-Team! No more React warnings for you, fool!
Also applies to: 123-125, 213-215, 348-350, 399-401, 420-422, 478-480, 490-492, 497-499, 540-542, 591-593, 613-615, 620-622
137-148: Verify react-tether upgrade & package.json presence — I pity the fool who doesn't!
- Tests assert react-tether v3 API (TetherComponent props like renderElementTo, attachment) — looks correct.
- package.json not found at repo root; cannot confirm installed react-tether version or that lockfile was updated.
- GH vulnerability query returned no advisories; changelog for 3.0.3 only adds @types/tether and lists no breaking changes.
- Action: Add or point to the dependency manifest (package.json or lockfile: package-lock.json / yarn.lock / pnpm-lock.yaml) showing [email protected] so the upgrade and lockfile can be verified.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (2)
224-236: Drop the redundant check inside renderElement, foolYou already guard on isConfirmingDelete via the ternary. No need to also do
isConfirmingDelete &&around DeleteConfirmation.- return isConfirmingDelete ? ( - <div ref={ref} style={{ display: 'inline-block' }}> - {isConfirmingDelete && ( - <DeleteConfirmation - data-resin-component={ACTIVITY_TARGETS.ANNOTATION_OPTIONS} - isOpen={isConfirmingDelete} - message={messages.annotationActivityDeletePrompt} - onDeleteCancel={handleDeleteCancel} - onDeleteConfirm={handleDeleteConfirm} - /> - )} - </div> - ) : null; + return isConfirmingDelete ? ( + <div ref={ref} style={{ display: 'inline-block' }}> + <DeleteConfirmation + data-resin-component={ACTIVITY_TARGETS.ANNOTATION_OPTIONS} + isOpen={isConfirmingDelete} + message={messages.annotationActivityDeletePrompt} + onDeleteCancel={handleDeleteCancel} + onDeleteConfirm={handleDeleteConfirm} + /> + </div> + ) : null;
201-208: Explicitly mount overlay to body to avoid clipping/z-index surprisesI pity the fool who lets modals get clipped by overflow: hidden. Add renderElementTo to be explicit and consistent with other components.
- <TetherComponent - {...tetherProps} + <TetherComponent + {...tetherProps} + renderElementTo={document.body}Please sanity-check in scrolling containers that the confirmation isn’t clipped.
src/elements/content-sidebar/activity-feed/task-new/Task.js (1)
328-339: Close the confirm immediately on delete, suckaKeep the UI snappy and consistent with AnnotationActivity by hiding the dialog before firing onDelete. Otherwise, a failed delete can strand the confirm open.
Update handleDeleteConfirm (outside this hunk):
handleDeleteConfirm = (): void => { const { id, onDelete, permissions } = this.props; this.setState({ isConfirmingDelete: false }); if (onDelete) { onDelete({ id, permissions }); } };src/components/tooltip/Tooltip.tsx (1)
344-384: Fix ref callback typings for renderTarget/renderElementTether passes a ref callback; typing as RefObject is misleading. Use React.Ref (callback or object).
- const renderTooltip = (ref: React.RefObject<HTMLDivElement>) => { + const renderTooltip = (ref: React.Ref<HTMLDivElement>) => {- renderTarget={(ref: React.RefObject<HTMLDivElement>) => { + renderTarget={(ref: React.Ref<HTMLDivElement>) => {Also applies to: 391-398
src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (1)
103-111: Localize the aria-label, foolUse intl formatting instead of defaultMessage to keep a11y strings translated.
- <MenuItem - aria-label={messages.commentDeleteMenuItem.defaultMessage} + <MenuItem + aria-label={intl.formatMessage(messages.commentDeleteMenuItem)}Add (outside this hunk):
// imports import { useIntl } from 'react-intl'; // inside component const intl = useIntl();src/elements/content-sidebar/activity-feed/comment/Comment.js (1)
246-257: Hide the confirm on delete to avoid stale UIMatch AnnotationActivity behavior: close the dialog before calling onDelete, or it can hang on failures.
Update (outside this hunk):
handleDeleteConfirm = (): void => { const { id, onDelete, permissions } = this.props; this.setState({ isConfirmingDelete: false }); onDelete({ id, permissions }); this.selectComment(false); };src/components/radar/RadarAnimation.tsx (2)
112-124: Type constraints, fool — don’t leave it implicitGive constraints a concrete type from TetherProps to catch mistakes at compile time.
Apply this diff:
- const constraints = []; + const constraints: NonNullable<TetherProps['constraints']> = [];
126-131: Don’t point aria-describedby at a ghostWhen hidden, the describedby target won’t exist or is decorative. Gate it to avoid noisy a11y for screen readers.
Apply this diff:
- const referenceElement = React.cloneElement(child as React.ReactElement, { - 'aria-describedby': this.radarAnimationID, - }); + const referenceElement = React.cloneElement(child as React.ReactElement, { + 'aria-describedby': isShown ? this.radarAnimationID : undefined, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/components/context-menu/ContextMenu.tsx(1 hunks)src/components/dropdown-menu/DropdownMenu.js(1 hunks)src/components/flyout/Flyout.js(1 hunks)src/components/radar/RadarAnimation.tsx(4 hunks)src/components/tooltip/Tooltip.tsx(3 hunks)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js(1 hunks)src/elements/content-sidebar/activity-feed/app-activity/AppActivity.js(1 hunks)src/elements/content-sidebar/activity-feed/comment/Comment.js(1 hunks)src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx(1 hunks)src/elements/content-sidebar/activity-feed/task-new/Task.js(1 hunks)src/features/left-sidebar/LeftSidebarLinkCallout.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/flyout/Flyout.js
- src/features/left-sidebar/LeftSidebarLinkCallout.js
- src/elements/content-sidebar/activity-feed/app-activity/AppActivity.js
- src/components/dropdown-menu/DropdownMenu.js
- src/components/context-menu/ContextMenu.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T17:50:47.187Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4228
File: src/elements/content-sidebar/activity-feed/comment/Comment.js:147-148
Timestamp: 2025-08-19T17:50:47.187Z
Learning: In the box-ui-elements codebase, ActivityThreadReplyForm.js and AnnotationThreadCreate.js component files exist but are not actively used/imported anywhere, so they don't need updates for prop propagation.
Applied to files:
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js
🧬 Code graph analysis (6)
src/components/radar/RadarAnimation.tsx (2)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/components/tooltip/Tooltip.tsx (2)
src/components/flyout/Flyout.js (2)
tetherProps(454-463)tetherPosition(392-392)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (4)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenuWrapper.js (4)
isConfirmingDelete(39-39)handleMenuClose(62-66)handleDeleteCancel(46-49)handleDeleteConfirm(41-42)src/elements/common/interactionTargets.js (2)
ACTIVITY_TARGETS(34-59)ACTIVITY_TARGETS(34-59)src/elements/content-sidebar/activity-feed/annotations/messages.js (1)
messages(3-50)
src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (3)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (6)
handleMenuClose(99-99)isConfirmingDelete(63-63)isResolved(84-84)canDelete(79-79)handleDeleteCancel(87-87)handleDeleteConfirm(88-91)src/elements/content-sidebar/activity-feed/comment/Comment.js (4)
canResolve(164-164)isResolved(169-169)canEdit(163-163)canDelete(162-162)src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenuWrapper.js (3)
handleStatusUpdate(69-73)handleEditClick(56-60)handleDeleteClick(51-54)
src/elements/content-sidebar/activity-feed/task-new/Task.js (2)
src/elements/common/interactionTargets.js (2)
ACTIVITY_TARGETS(34-59)ACTIVITY_TARGETS(34-59)src/elements/content-sidebar/activity-feed/task-new/messages.js (1)
messages(9-188)
src/elements/content-sidebar/activity-feed/comment/Comment.js (2)
src/elements/content-sidebar/activity-feed/comment/BaseComment.js (4)
canResolve(143-143)isResolved(147-147)canEdit(142-142)canDelete(141-141)src/elements/content-sidebar/activity-feed/comment/messages.js (1)
messages(9-65)
⏰ 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). (3)
- GitHub Check: lint_test_build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (4)
src/components/radar/RadarAnimation.tsx (4)
60-63: Children must be a single element — good fix, suckaSolid change to React.ReactElement. I pity the fool who widens this back to ReactNode.
21-58: Ditching string literals for TetherPosition constants — niceLess typos, more clarity. That’s how you do it.
132-152: Use classes over className on Tether for consistency and correctnessAcross the repo (see Flyout), we set overlay classes via classes.element. className may not behave the same across react-tether versions. Be consistent, fool.
Apply this diff:
- const tetherProps: Pick< - TetherProps, - 'attachment' | 'targetAttachment' | 'constraints' | 'classPrefix' | 'enabled' - > & { - offset?: string; - className?: string; - } = { + const tetherProps: Pick< + TetherProps, + 'attachment' | 'targetAttachment' | 'constraints' | 'classPrefix' | 'enabled' | 'classes' + > & { + offset?: string; + } = { attachment, classPrefix: 'radar-animation', constraints, enabled: isShown, targetAttachment, }; if (tetherElementClassName) { - tetherProps.className = tetherElementClassName; + tetherProps.classes = { element: tetherElementClassName }; }Run this to confirm how Tether is used elsewhere and align:
3-3: Confirmed: TetherProps in [email protected] includes enabled, classPrefix, constraints, attachment, targetAttachment, offset, and classes.
I pity the fool who skips verification — README/changelog confirm react-tether passes upstream Tether options.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (1)
241-345: Keep the wrapper in sync after actYou’re flipping state inside
React.act, but you never refresh the Enzyme wrapper before asserting. That leaves you poking at the pre-update tree, so the DeleteConfirmation check can silently miss a regression. I pity the fool who trusts a stale wrapper—match the pattern you used elsewhere and update right after the act block.React.act(() => { wrapper.find(AnnotationActivityMenu).prop('onDelete')(); }); + wrapper.update(); expect(wrapper.exists(DeleteConfirmation));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
src/components/media/__tests__/__snapshots__/Media.test.tsx.snapis excluded by!**/*.snapsrc/components/radar/__tests__/__snapshots__/RadarAnimation.test.tsx.snapis excluded by!**/*.snapsrc/components/sidebar-toggle-button/__tests__/__snapshots__/SidebarToggleButton.test.js.snapis excluded by!**/*.snapsrc/features/pagination/__tests__/__snapshots__/MarkerBasedPagination.test.js.snapis excluded by!**/*.snapsrc/features/pagination/__tests__/__snapshots__/OffsetBasedPagination.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
src/components/tooltip/Tooltip.scss(1 hunks)src/components/tooltip/Tooltip.tsx(5 hunks)src/elements/content-sidebar/SidebarNavButton.js(3 hunks)src/elements/content-sidebar/SidebarNavButton.scss(4 hunks)src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js(10 hunks)src/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.js(2 hunks)src/elements/content-sidebar/activity-feed/comment/Comment.js(2 hunks)src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js(6 hunks)src/elements/content-sidebar/activity-feed/task-new/__tests__/Task.test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sidebar/activity-feed/app-activity/tests/AppActivity.test.js
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-03T18:10:42.760Z
Learnt from: fpan225
PR: box/box-ui-elements#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/SidebarNavButton.scsssrc/elements/content-sidebar/SidebarNavButton.js
📚 Learning: 2025-09-03T18:10:29.467Z
Learnt from: fpan225
PR: box/box-ui-elements#4239
File: src/elements/content-sidebar/SidebarNav.js:94-106
Timestamp: 2025-09-03T18:10:29.467Z
Learning: In the ContentSidebar CustomSidebarPanel API, the navButtonProps spread should come after explicit data-* attributes to allow users to override analytics tracking attributes like data-resin-target, data-target-id, and data-testid when needed. This is intentional API design for flexibility.
Applied to files:
src/elements/content-sidebar/SidebarNavButton.scsssrc/elements/content-sidebar/SidebarNavButton.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sidebar/SidebarNavButton.js
📚 Learning: 2025-08-19T17:50:47.187Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4228
File: src/elements/content-sidebar/activity-feed/comment/Comment.js:147-148
Timestamp: 2025-08-19T17:50:47.187Z
Learning: In the box-ui-elements codebase, ActivityThreadReplyForm.js and AnnotationThreadCreate.js component files exist but are not actively used/imported anywhere, so they don't need updates for prop propagation.
Applied to files:
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js
🧬 Code graph analysis (4)
src/elements/content-sidebar/activity-feed/comment/Comment.js (2)
src/elements/content-sidebar/activity-feed/comment/BaseComment.js (4)
canResolve(143-143)isResolved(147-147)canEdit(142-142)canDelete(141-141)src/elements/content-sidebar/activity-feed/comment/messages.js (1)
messages(9-65)
src/components/tooltip/Tooltip.tsx (2)
src/components/flyout/Flyout.js (2)
tetherProps(454-463)tetherPosition(392-392)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/elements/content-sidebar/SidebarNavButton.js (2)
src/elements/content-sidebar/__tests__/withSidebarAnnotations.rtl.test.js (1)
internalSidebarNavigationHandler(55-55)src/utils/dom.js (1)
isLeftClick(79-81)
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (1)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
AnnotationActivity(48-98)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarNavButton.js
[error] 14-18: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint_test_build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (9)
src/elements/content-sidebar/activity-feed/comment/Comment.js (1)
184-259: Solid tether migration, fool!Digging this renderTarget/renderElement ref handoff—keeps the dropdown anchored while popping that DeleteConfirmation exactly when needed. I pity the fool who forgets to guard the confirm state, but you kept it tight. Respect.
src/elements/content-sidebar/activity-feed/task-new/__tests__/Task.test.js (2)
301-314: Mount-based permission checks look righteousI pity the fool who forgets to pop open that actions menu—this mount flow keeps the assertions honest with the tether upgrade. Nice work.
317-330: Menu gating logic stays toughClicking through the actions menu before asserting keeps these permission expectations rock solid, sucka. Approved.
src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js (3)
122-147: Scoped menu assertions dialed inSwitching to mount and cracking the menu only when it should appear? That’s precision, fool. These checks match the new overlay behavior perfectly.
167-185: Resolve menu coverage stays fierceOpening the actions menu before hunting for resolve options keeps these expectations legit. Mr. T approves.
205-223: Status-driven menu items still mean businessTying the resolve/unresolve items to status after the click fits the new tether flow just right, chump.
src/elements/content-sidebar/SidebarNavButton.scss (1)
62-64: Block-level nav target keeps the layout diesel-strongNice job giving the tether wrapper a block display. I pity the fool who lets that sidebar button collapse.
src/components/tooltip/Tooltip.scss (1)
228-230: Inline-block target for smooth tetheringLocking the wrapper to inline-block keeps the tooltip target steady. I pity the fool who skips that polish.
src/components/tooltip/Tooltip.tsx (1)
390-394: Drop the duplicate renderElementTo propYou already tuck
renderElementTointotetherProps, then pass it again right here. The second assignment just stomps the first, and it’s the same value anyway. I pity the fool who leaves redundant props behind.- <TetherComponent - {...tetherProps} - ref={this.tetherRef} - renderElementTo={bodyEl ?? (document.body as HTMLElement)} + <TetherComponent + {...tetherProps} + ref={this.tetherRef}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/elements/content-sidebar/activity-feed/comment/Comment.js (2)
189-196: Anchor wrapper should be inline-block to avoid layout shift.That bare div becomes block-level and can nudge layout. Make it inline-block, sucka.
Apply this diff:
- renderTarget={ref => ( - <div ref={ref}> + renderTarget={ref => ( + <div ref={ref} style={{ display: 'inline-block' }}>
247-258: Tethered element is gated; isOpen prop is redundant.You already conditionally render via renderElement. Dropping isOpen reduces prop churn, fool. Keep it only if DeleteConfirmation needs it for transitions.
Apply this diff:
- <DeleteConfirmation + <DeleteConfirmation data-resin-component={ACTIVITY_TARGETS.COMMENT_OPTIONS} - isOpen={isConfirmingDelete} message={messages.commentDeletePrompt} onDeleteCancel={this.handleDeleteCancel} onDeleteConfirm={this.handleDeleteConfirm} />src/elements/content-sidebar/SidebarNavButton.js (1)
60-66: Make hasExtraParams a real boolean and guard against falsy-but-valid idsTruthiness can bite if any id is 0 or an empty string. Tighten it up, sucka.
Apply:
- const hasExtraParams = - internalSidebarNavigation && - (internalSidebarNavigation.versionId || - internalSidebarNavigation.activeFeedEntryType || - internalSidebarNavigation.activeFeedEntryId || - internalSidebarNavigation.fileVersionId); + const hasExtraParams = + !!internalSidebarNavigation && + (internalSidebarNavigation.versionId != null || + internalSidebarNavigation.activeFeedEntryType != null || + internalSidebarNavigation.activeFeedEntryId != null || + internalSidebarNavigation.fileVersionId != null);src/components/menu/MenuItem.tsx (1)
73-73: Verify list semantics with RadarAnimation wrapperWrapping an
can introduce a non- ancestor under . Browsers forgive, validators don’t. Ensure RadarAnimation’s target wrapper sits inside the
- or the wrapper is role="presentation" and display:block to avoid layout/accessibility surprises, fool.
src/components/dropdown-menu/DropdownMenu.js (1)
270-271: Unify Tether styling approach for consistency.Across the codebase (e.g., Flyout), y’all use classes.element for Tether styling. Consider aligning here for predictability.
Apply this diff:
- className={classNames({ 'bdl-DropdownMenu--responsive': isResponsive }, className)} + classes={{ element: classNames({ 'bdl-DropdownMenu--responsive': isResponsive }, className) }}src/components/radar/RadarAnimation.tsx (1)
132-135: Only add aria-describedby when shown.Don’t point ATs to a missing node, fool. Gate the ARIA hook by isShown.
- const referenceElement = React.cloneElement(child as React.ReactElement, { - 'aria-describedby': this.radarAnimationID, - }); + const referenceElement = React.cloneElement(child as React.ReactElement, { + ...(isShown ? { 'aria-describedby': this.radarAnimationID } : {}), + });src/components/tooltip/Tooltip.tsx (1)
318-330: Add enabled to Tether to skip work when hidden.Let Tether short‑circuit when not shown. Save cycles, fool.
const tetherProps: Pick< TetherProps, 'attachment' | 'targetAttachment' | 'constraints' | 'renderElementTo' | 'classPrefix' > & { offset?: string; className?: string; } = { attachment: tetherPosition.attachment, classPrefix: 'tooltip', constraints, targetAttachment: tetherPosition.targetAttachment, renderElementTo: bodyEl, + // Avoid Tether overhead when the tooltip is hidden + // @ts-expect-error: enabled exists on TetherProps runtime; Pick omits it + enabled: showTooltip, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
src/components/footer-indicator/__tests__/__snapshots__/FooterIndicator.test.tsx.snapis excluded by!**/*.snapsrc/components/media/__tests__/__snapshots__/Media.test.tsx.snapis excluded by!**/*.snapsrc/components/menu/__tests__/__snapshots__/MenuItem.test.tsx.snapis excluded by!**/*.snapsrc/components/select/__tests__/__snapshots__/Select.test.js.snapis excluded by!**/*.snapsrc/features/collapsible-sidebar/__tests__/__snapshots__/CollapsibleSidebarMenuItem.test.js.snapis excluded by!**/*.snapsrc/features/pagination/__tests__/__snapshots__/OffsetBasedPagination.test.js.snapis excluded by!**/*.snapsrc/features/sandbox-banner/__tests__/__snapshots__/SandboxBanner.test.tsx.snapis excluded by!**/*.snapsrc/features/unified-share-modal/__tests__/__snapshots__/SharedLinkSection.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
src/components/draft-js-editor/DraftJSEditor.js(1 hunks)src/components/draft-js-editor/DraftJSEditor.scss(1 hunks)src/components/dropdown-menu/DropdownMenu.js(3 hunks)src/components/dropdown-menu/DropdownMenu.scss(1 hunks)src/components/footer-indicator/FooterIndicator.scss(1 hunks)src/components/footer-indicator/FooterIndicator.tsx(1 hunks)src/components/media/Media.scss(1 hunks)src/components/media/MediaMenu.tsx(1 hunks)src/components/menu/MenuItem.scss(1 hunks)src/components/menu/MenuItem.tsx(2 hunks)src/components/radar/RadarAnimation.scss(1 hunks)src/components/radar/RadarAnimation.stories.tsx(1 hunks)src/components/radar/RadarAnimation.tsx(5 hunks)src/components/select/Select.js(1 hunks)src/components/tooltip/Tooltip.tsx(5 hunks)src/elements/content-sidebar/SidebarNavButton.js(3 hunks)src/elements/content-sidebar/activity-feed/comment/Comment.js(2 hunks)src/features/collapsible-sidebar/CollapsibleSidebar.scss(1 hunks)src/features/collapsible-sidebar/CollapsibleSidebarMenuItem.js(1 hunks)src/features/sandbox-banner/SandboxBanner.tsx(1 hunks)src/features/unified-share-modal/SharedLinkSection.js(3 hunks)src/features/unified-share-modal/UnifiedShareModal.scss(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/menu/MenuItem.scss
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-16T14:53:03.538Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4275
File: src/components/form-elements/draft-js-mention-selector/index.js:4-4
Timestamp: 2025-09-16T14:53:03.538Z
Learning: The createMentionSelectorState export in src/components/form-elements/draft-js-mention-selector/index.js was confirmed to be internal-only within the box-ui-elements package, with no external references or documentation, making the breaking rename to createMentionTimestampSelectorState safe.
Applied to files:
src/components/draft-js-editor/DraftJSEditor.js
📚 Learning: 2025-09-09T21:43:34.180Z
Learnt from: bxie-box
PR: box/box-ui-elements#4271
File: src/features/classification/README.md:67-67
Timestamp: 2025-09-09T21:43:34.180Z
Learning: In the Classification component's aiClassificationReason prop, TooltipProvider is only required when the citations array is empty (shows tooltip UI), not when citations are present (shows card on hover UI).
Applied to files:
src/components/draft-js-editor/DraftJSEditor.jssrc/components/footer-indicator/FooterIndicator.tsx
📚 Learning: 2025-09-03T18:10:42.760Z
Learnt from: fpan225
PR: box/box-ui-elements#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/SidebarNavButton.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sidebar/SidebarNavButton.js
📚 Learning: 2025-09-03T18:10:29.467Z
Learnt from: fpan225
PR: box/box-ui-elements#4239
File: src/elements/content-sidebar/SidebarNav.js:94-106
Timestamp: 2025-09-03T18:10:29.467Z
Learning: In the ContentSidebar CustomSidebarPanel API, the navButtonProps spread should come after explicit data-* attributes to allow users to override analytics tracking attributes like data-resin-target, data-target-id, and data-testid when needed. This is intentional API design for flexibility.
Applied to files:
src/elements/content-sidebar/SidebarNavButton.js
🧬 Code graph analysis (6)
src/components/tooltip/Tooltip.tsx (2)
src/components/flyout/Flyout.js (2)
tetherProps(454-463)tetherPosition(392-392)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/components/footer-indicator/FooterIndicator.tsx (1)
src/components/tooltip/index.ts (1)
TooltipPosition(5-5)
src/elements/content-sidebar/activity-feed/comment/Comment.js (3)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (3)
isConfirmingDelete(63-63)isResolved(84-84)canDelete(79-79)src/elements/content-sidebar/activity-feed/comment/BaseComment.js (4)
canResolve(143-143)isResolved(147-147)canEdit(142-142)canDelete(141-141)src/elements/content-sidebar/activity-feed/comment/messages.js (1)
messages(9-65)
src/features/unified-share-modal/SharedLinkSection.js (1)
src/features/unified-share-modal/messages.js (1)
messages(3-581)
src/components/radar/RadarAnimation.tsx (2)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/elements/content-sidebar/SidebarNavButton.js (2)
src/elements/content-sidebar/__tests__/withSidebarAnnotations.rtl.test.js (1)
internalSidebarNavigationHandler(55-55)src/utils/dom.js (1)
isLeftClick(79-81)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarNavButton.js
[error] 14-18: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint_test_build
- GitHub Check: Summary
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (31)
src/features/unified-share-modal/UnifiedShareModal.scss (3)
50-52: Block target keeps layout tightCleans up the wrapper alignment. I pity the fool who lets tooltip anchors collapse.
54-56: Flex prop nailed itKeeps the FTUX toggle stretching with the row. I pity the fool who leaves dead space in flex land.
213-215: Span width fix looks righteousMakes that toggle row behave with the new tooltip wrapper. I pity the fool who forgets the full-width span.
src/elements/content-sidebar/activity-feed/comment/Comment.js (2)
66-67: New onSelect defaultProp is solid, fool.Good addition; keeps callers optional and internal logic safe.
185-187: Verify.bcs-Comment-deleteConfirmationModallands on the portal container
Inspect the rendered tethered node to ensure the className is applied where the CSS is targeting—if it’s on the wrong element, move it to the inner content wrapper. I pity the fool who doesn’t!src/components/radar/RadarAnimation.scss (1)
121-123: Solid target wrapper hook for RadarAnimationInline-block is the right call for a compact anchor. I pity the fool who ships tether targets without a stable wrapper!
src/elements/content-sidebar/SidebarNavButton.js (4)
14-18: Biome warning looks bogus for Flow’simport typeThis is valid Flow in .js with
@flow. Make sure Biome is configured to parse Flow or is scoped away from Flow files, fool.
74-80: Refactor/readability tweak is cleanSplit args and trailing comma read better. No behavior change. That’s tight.
85-90: Parity achieved: wrapper on routerDisabled pathGood catch adding targetWrapperClassName here too. Now both paths render the same wrapper. I pity the fool who leaves branches inconsistent.
137-142: Consistent tooltip target wrapperMatching class in the router path keeps layout steady. Approved.
src/components/draft-js-editor/DraftJSEditor.scss (1)
20-22: Good public hook for tooltip/tether targetBlock display keeps the editor’s target full-width. Looks legit.
src/components/media/Media.scss (1)
44-46: Menu target wrapper class is on pointBlock display prevents width collapse in the media row. Ship it.
src/components/footer-indicator/FooterIndicator.scss (1)
39-41: Wrapper height 100% is the right moveKeeps the tooltip target spanning the indicator. No janky hit area here, fool.
src/components/footer-indicator/FooterIndicator.tsx (1)
15-18: Tooltip now wired with a wrapper classMatches the styling hook in SCSS. Clean and consistent.
src/components/radar/RadarAnimation.stories.tsx (1)
61-65: Story tweak looks fineInline style number maps to px; offset scenario still covered. All good.
src/components/menu/MenuItem.tsx (1)
7-8: Importing MenuItem.scss is appropriatePulls in the new radar target class. Tight coupling where it belongs.
src/features/unified-share-modal/SharedLinkSection.js (1)
530-534: Solid tooltip target hook.Dialing in that
targetWrapperClassNamekeeps the FTUX toggle styling on lock—nice consistency play. I pity the fool who forgets to anchor their tooltips this cleanly.src/components/draft-js-editor/DraftJSEditor.js (1)
129-135: Error tooltip wrapper looks sharp.Anchoring the DraftJS error tooltip with its own target class keeps the layout steady—no more wobbling targets on my watch. I pity the fool who overlooks that polish.
src/features/collapsible-sidebar/CollapsibleSidebar.scss (1)
116-118: Sidebar tooltip target styling is tight.That combined selector locks the tooltip target into place so those menu items behave whether the nav is expanded or not. I pity the fool who lets sidebar tooltips drift.
src/components/dropdown-menu/DropdownMenu.scss (1)
22-24: Dropdown target hook lands perfectly.Giving the tether target an inline-block wrapper keeps the trigger predictable without messing with menu physics. I pity the fool who leaves dropdown anchors undefined.
src/components/select/Select.js (1)
66-77: Info tooltip refactor stays disciplined.Routing the icon container styling through
targetWrapperClassNamekeeps Tooltip’s new structure happy while preserving the click-to-toggle behavior. I pity the fool who’d juggle extra wrapper spans for nothing.src/components/media/MediaMenu.tsx (1)
41-46: Menu dropdown props read crystal clear.Making the alignment and scroll constraints explicit while tagging the target wrapper keeps this media menu mean and lean. I pity the fool who buries those defaults in spread props.
src/features/sandbox-banner/SandboxBanner.tsx (1)
13-15: Banner tooltip wiring is clean.Letting Tooltip own the outer wrapper while reusing the banner text both visually and in the tooltip keeps things DRY. I pity the fool who’d duplicate markup for this little badge.
src/features/collapsible-sidebar/CollapsibleSidebarMenuItem.js (1)
197-206: Sidebar tooltip target class hits the mark.That additional wrapper class lets the SCSS do its job without touching the render logic. I pity the fool who leaves overflow tooltips without a proper anchor.
src/components/dropdown-menu/DropdownMenu.js (2)
16-17: New targetWrapperClassName prop — solid.Nice hook for styling the target wrapper. I pity the fool who can’t find their target!
274-288: renderTarget/renderElement migration — looks good.Body mounting + wrapper classes are clean. Good guard on isOpen. Respect.
src/components/radar/RadarAnimation.tsx (4)
24-58: Position mapping via TetherPosition — tight.Clear and consistent mappings. I like it.
63-67: Children typing and target wrapper — legit.Enforcing ReactElement and adding targetWrapperClassName is the right move, sucka.
136-148: Don’t render hidden radar at all.Gate the tethered element on isShown to avoid invisible DOM for a11y and hit‑testing.
- renderElement={ref => ( - <div ref={ref} className={`radar ${className}`} id={this.radarAnimationID} {...rest}> + renderElement={ref => + isShown ? ( + <div + ref={ref} + className={`radar ${className}`} + id={this.radarAnimationID} + aria-hidden="true" + {...rest} + > <div className="radar-dot" /> <div className="radar-circle" /> - </div> - )} + </div> + ) : null + }Also applies to: 167-173
159-166: Target wrapper render — clean.Inline-block wrapper with optional class is clutch.
src/components/tooltip/Tooltip.tsx (1)
105-107: New targetWrapperClassName prop — thumbs up.Styling hook is handy. Keep it.
jfox-box
force-pushed
the
react-tether-upgrade-3.0.3
branch
from
a8d53ed to
2d0755f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (1)
1-2: Fix the bad act import, fool!
React don't shipactfrom the main package. Pulling it from'react'leaves you withundefinedand every test blows up before it even runs. Snag it from'react-dom/test-utils'so the menu tests can strut proud.-import React, { act } from 'react'; +import React from 'react'; +import { act } from 'react-dom/test-utils';
🧹 Nitpick comments (2)
src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js (1)
48-82: Don’t leave mounted wrappers hanging, fool!Line 54 and Line 72: Mounting sprays portal nodes into
document.body; without an explicitwrapper.unmount()those suckers linger across tests and can bite us later. Wrap the assertions in a try/finally or at least unmount once the clicks are done.Apply this diff in both mount-based tests:
- const tetherComponent = findTetherComponent(wrapper); - const btn = tetherComponent.find('.nav-link-callout-close-button').first(); - btn.simulate('click'); + try { + const tetherComponent = findTetherComponent(wrapper); + const btn = tetherComponent.find('.nav-link-callout-close-button').first(); + btn.simulate('click'); + } finally { + wrapper.unmount(); + }(and mirror the same pattern in the navLinkClassName test)
</blockquote></details> <details> <summary>src/components/context-menu/__tests__/ContextMenu.test.tsx (1)</summary><blockquote> `84-103`: **Clean up that mounted menu, fool** You crack this menu open, but you never shut it down. ContextMenu wires up document-level listeners when it’s open, so leaving the wrapper mounted after the assertions risks leaking those handlers into the rest of the suite. I pity the fool who debugs that kind of bleed-over! Move to unmount (or otherwise close) before you exit the test. ```diff expect(menu.prop('initialFocusIndex')).toEqual(null); + wrapper.unmount();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
src/components/footer-indicator/__tests__/__snapshots__/FooterIndicator.test.tsx.snapis excluded by!**/*.snapsrc/components/media/__tests__/__snapshots__/Media.test.tsx.snapis excluded by!**/*.snapsrc/components/menu/__tests__/__snapshots__/MenuItem.test.tsx.snapis excluded by!**/*.snapsrc/components/radar/__tests__/__snapshots__/RadarAnimation.test.tsx.snapis excluded by!**/*.snapsrc/components/select/__tests__/__snapshots__/Select.test.js.snapis excluded by!**/*.snapsrc/components/sidebar-toggle-button/__tests__/__snapshots__/SidebarToggleButton.test.js.snapis excluded by!**/*.snapsrc/components/tooltip/__tests__/__snapshots__/Tooltip.test.tsx.snapis excluded by!**/*.snapsrc/elements/content-sidebar/activity-feed/app-activity/__tests__/__snapshots__/AppActivity.test.js.snapis excluded by!**/*.snapsrc/elements/content-sidebar/activity-feed/comment/__tests__/__snapshots__/Comment.test.js.snapis excluded by!**/*.snapsrc/elements/content-sidebar/activity-feed/task-new/__tests__/__snapshots__/Task.test.js.snapis excluded by!**/*.snapsrc/features/collapsible-sidebar/__tests__/__snapshots__/CollapsibleSidebarMenuItem.test.js.snapis excluded by!**/*.snapsrc/features/left-sidebar/__tests__/__snapshots__/LeftSidebarLinkCallout.test.js.snapis excluded by!**/*.snapsrc/features/pagination/__tests__/__snapshots__/MarkerBasedPagination.test.js.snapis excluded by!**/*.snapsrc/features/pagination/__tests__/__snapshots__/OffsetBasedPagination.test.js.snapis excluded by!**/*.snapsrc/features/sandbox-banner/__tests__/__snapshots__/SandboxBanner.test.tsx.snapis excluded by!**/*.snapsrc/features/unified-share-modal/__tests__/__snapshots__/SharedLinkSection.test.js.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (48)
.flowconfig(1 hunks)package.json(2 hunks)src/components/button-group/ButtonGroup.scss(3 hunks)src/components/checkbox/Checkbox.scss(1 hunks)src/components/context-menu/ContextMenu.tsx(1 hunks)src/components/context-menu/__tests__/ContextMenu.test.tsx(9 hunks)src/components/draft-js-editor/DraftJSEditor.js(1 hunks)src/components/draft-js-editor/DraftJSEditor.scss(1 hunks)src/components/dropdown-menu/DropdownMenu.js(3 hunks)src/components/dropdown-menu/DropdownMenu.scss(1 hunks)src/components/dropdown-menu/__tests__/DropdownMenu.test.js(17 hunks)src/components/flyout/Flyout.js(1 hunks)src/components/flyout/__tests__/Flyout.test.js(20 hunks)src/components/footer-indicator/FooterIndicator.scss(1 hunks)src/components/footer-indicator/FooterIndicator.tsx(1 hunks)src/components/media/Media.scss(1 hunks)src/components/media/MediaMenu.tsx(1 hunks)src/components/menu/MenuItem.scss(1 hunks)src/components/menu/MenuItem.tsx(2 hunks)src/components/pill-selector-dropdown/__tests__/PillSelector.test.js(16 hunks)src/components/radar/RadarAnimation.scss(1 hunks)src/components/radar/RadarAnimation.stories.tsx(1 hunks)src/components/radar/RadarAnimation.tsx(5 hunks)src/components/radar/__tests__/RadarAnimation.test.tsx(2 hunks)src/components/select/Select.js(1 hunks)src/components/text-area/__tests__/TextArea.test.js(2 hunks)src/components/text-input/__tests__/TextInput.test.js(1 hunks)src/components/tooltip/Tooltip.scss(1 hunks)src/components/tooltip/Tooltip.tsx(5 hunks)src/components/tooltip/__tests__/Tooltip.test.tsx(14 hunks)src/elements/content-sidebar/SidebarNavButton.js(3 hunks)src/elements/content-sidebar/SidebarNavButton.scss(4 hunks)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js(1 hunks)src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js(10 hunks)src/elements/content-sidebar/activity-feed/app-activity/AppActivity.js(1 hunks)src/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.js(2 hunks)src/elements/content-sidebar/activity-feed/comment/Comment.js(2 hunks)src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js(6 hunks)src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx(1 hunks)src/elements/content-sidebar/activity-feed/task-new/Task.js(1 hunks)src/elements/content-sidebar/activity-feed/task-new/__tests__/Task.test.js(3 hunks)src/features/collapsible-sidebar/CollapsibleSidebar.scss(1 hunks)src/features/collapsible-sidebar/CollapsibleSidebarMenuItem.js(1 hunks)src/features/left-sidebar/LeftSidebarLinkCallout.js(1 hunks)src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js(3 hunks)src/features/sandbox-banner/SandboxBanner.tsx(1 hunks)src/features/unified-share-modal/SharedLinkSection.js(3 hunks)src/features/unified-share-modal/UnifiedShareModal.scss(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .flowconfig
🚧 Files skipped from review as they are similar to previous changes (23)
- src/components/text-input/tests/TextInput.test.js
- src/components/flyout/tests/Flyout.test.js
- src/elements/content-sidebar/activity-feed/comment/Comment.js
- src/features/unified-share-modal/UnifiedShareModal.scss
- src/features/collapsible-sidebar/CollapsibleSidebar.scss
- src/features/collapsible-sidebar/CollapsibleSidebarMenuItem.js
- src/components/text-area/tests/TextArea.test.js
- src/components/media/MediaMenu.tsx
- src/components/menu/MenuItem.scss
- src/components/button-group/ButtonGroup.scss
- src/elements/content-sidebar/activity-feed/task-new/tests/Task.test.js
- src/components/draft-js-editor/DraftJSEditor.scss
- src/components/flyout/Flyout.js
- src/elements/content-sidebar/activity-feed/app-activity/tests/AppActivity.test.js
- src/components/footer-indicator/FooterIndicator.tsx
- src/elements/content-sidebar/activity-feed/comment/tests/Comment.test.js
- src/components/draft-js-editor/DraftJSEditor.js
- src/components/menu/MenuItem.tsx
- src/components/radar/RadarAnimation.scss
- src/components/dropdown-menu/DropdownMenu.scss
- src/elements/content-sidebar/activity-feed/annotations/tests/AnnotationActivity.test.js
- src/components/checkbox/Checkbox.scss
- src/elements/content-sidebar/activity-feed/app-activity/AppActivity.js
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-09-15T17:04:28.279Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4275
File: src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js:259-260
Timestamp: 2025-09-15T17:04:28.279Z
Learning: When wrapping enzyme mount() tests with IntlProvider for class-based components, use wrapper.find('ComponentName') to access the actual component instance and state, since the wrapper becomes the provider instead of the component.
Applied to files:
src/components/radar/__tests__/RadarAnimation.test.tsxsrc/components/tooltip/__tests__/Tooltip.test.tsxsrc/components/context-menu/__tests__/ContextMenu.test.tsxsrc/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js
📚 Learning: 2025-09-03T18:10:42.760Z
Learnt from: fpan225
PR: box/box-ui-elements#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/SidebarNavButton.scsssrc/elements/content-sidebar/SidebarNavButton.js
📚 Learning: 2025-08-27T17:03:48.322Z
Learnt from: bxie-box
PR: box/box-ui-elements#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/components/tooltip/__tests__/Tooltip.test.tsx
📚 Learning: 2025-09-16T14:53:03.538Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4275
File: src/components/form-elements/draft-js-mention-selector/index.js:4-4
Timestamp: 2025-09-16T14:53:03.538Z
Learning: The createMentionSelectorState export in src/components/form-elements/draft-js-mention-selector/index.js was confirmed to be internal-only within the box-ui-elements package, with no external references or documentation, making the breaking rename to createMentionTimestampSelectorState safe.
Applied to files:
src/components/select/Select.js
📚 Learning: 2025-09-16T14:53:03.538Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4275
File: src/components/form-elements/draft-js-mention-selector/index.js:4-4
Timestamp: 2025-09-16T14:53:03.538Z
Learning: The createMentionSelectorState export in src/components/form-elements/draft-js-mention-selector/index.js was an internal-only API, not used by external consumers, so breaking changes during rename to createMentionTimestampSelectorState are acceptable.
Applied to files:
src/components/select/Select.js
📚 Learning: 2025-08-19T17:50:47.187Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4228
File: src/elements/content-sidebar/activity-feed/comment/Comment.js:147-148
Timestamp: 2025-08-19T17:50:47.187Z
Learning: In the box-ui-elements codebase, ActivityThreadReplyForm.js and AnnotationThreadCreate.js component files exist but are not actively used/imported anywhere, so they don't need updates for prop propagation.
Applied to files:
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js
📚 Learning: 2025-09-03T18:10:29.467Z
Learnt from: fpan225
PR: box/box-ui-elements#4239
File: src/elements/content-sidebar/SidebarNav.js:94-106
Timestamp: 2025-09-03T18:10:29.467Z
Learning: In the ContentSidebar CustomSidebarPanel API, the navButtonProps spread should come after explicit data-* attributes to allow users to override analytics tracking attributes like data-resin-target, data-target-id, and data-testid when needed. This is intentional API design for flexibility.
Applied to files:
src/elements/content-sidebar/SidebarNavButton.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sidebar/SidebarNavButton.js
🧬 Code graph analysis (12)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (1)
src/components/flyout/__tests__/Flyout.test.js (4)
renderWrapper(39-46)FakeButton(19-24)findTetherComponent(48-50)sandbox(7-7)
src/components/radar/__tests__/RadarAnimation.test.tsx (3)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (2)
renderWrapper(24-31)findTetherComponent(33-35)src/components/flyout/__tests__/Flyout.test.js (2)
renderWrapper(39-46)findTetherComponent(48-50)src/components/radar/RadarAnimation.tsx (1)
RadarAnimationProps(61-80)
src/components/radar/RadarAnimation.tsx (2)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (3)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (6)
handleMenuClose(99-99)isConfirmingDelete(63-63)isResolved(84-84)canDelete(79-79)handleDeleteCancel(87-87)handleDeleteConfirm(88-91)src/elements/content-sidebar/activity-feed/comment/Comment.js (4)
canResolve(165-165)isResolved(170-170)canEdit(164-164)canDelete(163-163)src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenuWrapper.js (3)
handleStatusUpdate(69-73)handleEditClick(56-60)handleDeleteClick(51-54)
src/components/context-menu/ContextMenu.tsx (2)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/components/context-menu/__tests__/ContextMenu.test.tsx (1)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (3)
FakeButton(12-12)FakeMenu(16-20)sandbox(8-8)
src/elements/content-sidebar/activity-feed/task-new/Task.js (2)
src/elements/common/interactionTargets.js (2)
ACTIVITY_TARGETS(34-59)ACTIVITY_TARGETS(34-59)src/elements/content-sidebar/activity-feed/task-new/messages.js (1)
messages(9-188)
src/components/tooltip/Tooltip.tsx (2)
src/components/flyout/Flyout.js (2)
tetherProps(454-463)tetherPosition(392-392)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js (2)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (1)
findTetherComponent(33-35)src/components/flyout/__tests__/Flyout.test.js (1)
findTetherComponent(48-50)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
src/elements/content-sidebar/activity-feed/annotations/messages.js (1)
messages(3-50)
src/features/unified-share-modal/SharedLinkSection.js (1)
src/features/unified-share-modal/messages.js (1)
messages(3-581)
src/elements/content-sidebar/SidebarNavButton.js (2)
src/elements/content-sidebar/__tests__/withSidebarAnnotations.rtl.test.js (1)
internalSidebarNavigationHandler(55-55)src/utils/dom.js (1)
isLeftClick(79-81)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarNavButton.js
[error] 14-18: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (33)
src/components/footer-indicator/FooterIndicator.scss (1)
39-42: Solid wrapper upgrade, sucka!
I pity the fool who leaves the tooltip target collapsing. Making it block-level with full height keeps that hover zone tight. No notes.src/components/media/Media.scss (1)
8-10: Looks solid, sucka! This block-level override keeps the dropdown target lining up right inside the media layout. I pity the fool who breaks that alignment.src/elements/content-sidebar/activity-feed/task-new/Task.js (2)
296-327: Solid menu anchor, fool!Inline-block wrapper keeps the tether target locked down, so the menu trigger stays stable. I pity the fool who messes with this anchor now.
328-340: Return is back in action, sucka!Hooking the block-bodied arrow up with an explicit return lets that delete confirmation actually show its face. I pity the fool who forgets to return JSX.
src/components/pill-selector-dropdown/__tests__/PillSelector.test.js (7)
204-205: Clean selection assertion, fool!Tightening that
isSelectedcheck keeps the intent crystal clear. I pity the fool who tries to misread this now!
281-281: Container click simulation nailed it, sucka!Pointing the click at
.bdl-PillSelectorlines up with the refocus logic—exactly where the handler lives. I pity the fool who overlooks that alignment!
308-362: KeyDown coverage stays mean, fool!Shifting the Backspace scenarios to the container keeps the tests marching with the new event surface. Every guard still gets flexed, and I pity the fool who thinks this lost coverage!
375-427: ArrowLeft hustle keeps pace, fool!Exercising the left-arrow flows through the container mirrors the runtime wiring after the tether shift. I pity the fool who doubts these navigation checks!
447-471: ArrowRight gauntlet still tough, fool!The right-arrow paths stay sharp even with the container-driven events. I pity the fool who tries to regress pill navigation now!
477-481: No-selection case still guarded, fool!The “no pill” scenario keeps its expectations intact after the event target swap. I pity the fool who misses this safety net!
493-504: OnRemove shortcuts look tight, fool!Those inline
onRemoveinvocations stay punchy without losing intent—immutable and plain arrays both covered. I pity the fool who questions these callbacks!src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (1)
57-128: Tether migration is tight, sucka.Listen up! This renderTarget/renderElement swap keeps the menu anchored and the delete confirmation bolted to the right spot. I pity the fool who doubts this upgrade.
src/features/left-sidebar/LeftSidebarLinkCallout.js (1)
73-86: Tether migration stays tight, fool!Listen up! Wrapping the target in that inline-block div keeps Tether measuring right, and the renderElement guard keeps the callout outta the DOM when it ain’t needed. I pity the fool who’d roll this back.
src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js (1)
7-9: Consistent tether finder, fool!Good hustle matching the shared
findTetherComponentpattern. I pity the fool who hunts for those portals by hand.src/components/tooltip/__tests__/Tooltip.test.tsx (1)
525-527: Toughen up the keydown spy check, fool!
Checking for one exact signature lets a sneaky keydown listener slide in with different options. Filter the spy calls so anykeydownregistration trips the test.- // Keydown listener should not be added since it's commented out - expect(addEventListenerSpy).not.toHaveBeenCalledWith('keydown', expect.anything(), true); - addEventListenerSpy.mockRestore(); + // Keydown listener should not be added since it's commented out + const keydownCalls = addEventListenerSpy.mock.calls.filter(([eventType]) => eventType === 'keydown'); + expect(keydownCalls.length).toBe(0); + addEventListenerSpy.mockRestore();src/components/tooltip/Tooltip.scss (1)
228-230: Inline-block wrapper keeps your tooltip target stout, fool.Nice touch making the target wrapper a dependable inline-block so the tether’s got a solid anchor every time.
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
201-238: Render split keeps that delete flyout locked in place, sucka.Breaking the tether into renderTarget/renderElement while guarding the confirmation flow keeps the menu anchored tight without layout jitters.
src/components/dropdown-menu/DropdownMenu.js (1)
276-287: Wrapper class hook makes this dropdown mighty, fool.The new target wrapper plus portal split plays real nice with react-tether v3 and keeps styling hooks clean.
src/components/radar/__tests__/RadarAnimation.test.tsx (1)
6-112: Mount helper got these tether assertions swole, sucker.Good move switching to a shared mount plus tether finder—means the offset and enabled checks hit the real rendered portal.
src/components/radar/RadarAnimation.tsx (1)
158-173: Refined tether usage keeps that radar blazing, fool.renderTarget/renderElement with the inline-block wrapper gives the radar a solid anchor and keeps the overlay pumping strong.
src/components/tooltip/Tooltip.tsx (1)
347-401: Fix the ref typing, fool.react-tether v3 hands us callback refs. Annotating these params as
React.RefObjectbreaks theTetherPropscontract and will fail the TypeScript check. Switch bothrenderTooltipandrenderTargetsignatures toReact.Ref<HTMLDivElement>(or drop the annotation) so the callback matches what Tether actually calls.- const renderTooltip = (ref: React.RefObject<HTMLDivElement>) => { + const renderTooltip = (ref: React.Ref<HTMLDivElement>) => { @@ - renderTarget={(ref: React.RefObject<HTMLDivElement>) => { + renderTarget={(ref: React.Ref<HTMLDivElement>) => {package.json (1)
269-269: Keep those tether versions in sync, sucka!
Bumping both dependency and peer dependency to ^3.0.3 keeps every consumer aligned with the new Tether contract. I pity the fool who leaves them drifting.Also applies to: 350-350
src/components/radar/RadarAnimation.stories.tsx (1)
61-65: Offset story still shines, fool!
That outer div gives the demo breathing room without touching the RadarAnimation API. I pity the fool who ships cramped previews.src/features/sandbox-banner/SandboxBanner.tsx (1)
13-15: Target wrapper hustle done right, sucka!
Shifting the styling hook onto Tooltip's wrapper keeps the DOM lean while preserving the banner text. I pity the fool who keeps extra divs hanging around.src/features/unified-share-modal/SharedLinkSection.js (3)
203-205: Clean destructuring, fool!
Breaking out the sharedLink props reads smoother and keeps Flow happy. I pity the fool who leaves that bundle tangled.
428-431: Props pull-up stays legit, sucka!
Reformatting the destructure keeps the long signature manageable with no behavior shift. I pity the fool who skips that clarity.
530-534: Wrapper class saves the layout, fool!
Passing targetWrapperClassName here makes the tether wrapper flex-friendly so the toggle keeps its width. I pity the fool who lets the FTUX toggle shrink.src/elements/content-sidebar/SidebarNavButton.scss (1)
62-64: Giving that nav target some muscle, sucka!
The new .bcs-NavButton-target rule keeps the tooltip wrapper full-width like the button. I pity the fool who leaves skinny hit areas.src/elements/content-sidebar/SidebarNavButton.js (4)
14-18: Flow types stay organized, sucka!
Reflowing the type import keeps the interface tidy while matching project style. I pity the fool who lets types sprawl.
60-66: Readable param check, sucka!
Spreading the extra param detection over multiple lines makes the condition obvious. I pity the fool who buries that logic.
74-80: Router mimic stays tight, fool!
Keeping the handler call multi-line improves Flow readability without touching behavior. I pity the fool who skims past navigation updates.
85-90: Consistent tooltip wrappers, fool!
Mirroring targetWrapperClassName across both nav paths keeps the layout steady whether the router is live or mocked. I pity the fool who forgets parity.Also applies to: 137-142
src/components/select/Select.js (1)
66-74: Tooltip wrapper swap done right, fool!
Handing className control to targetWrapperClassName keeps the button styling intact with the new react-tether API. I pity the fool who leaves stale wrappers.
greg-in-a-box
changed the title
greg-in-a-box
changed the title
jfox-box
force-pushed
the
react-tether-upgrade-3.0.3
branch
from
2d0755f to
7dccf6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/components/pill-selector-dropdown/__tests__/PillSelector.test.js (3)
321-325: Fix the bogus sinon mocks before this test faceplants.I pity the fool who hands
sandbox.mock().never()topreventDefaultandstopPropagation. That returns a mock controller, not a callable function, so hittingevent.preventDefault()blows up with “is not a function.” Swap insandbox.stub().never()(or a real spy) so the handler stays callable and the expectation sticks.- wrapper.find('.bdl-PillSelector').simulate('keyDown', { - key: 'Backspace', - preventDefault: sandbox.mock().never(), - stopPropagation: sandbox.mock().never(), - }); + wrapper.find('.bdl-PillSelector').simulate('keyDown', { + key: 'Backspace', + preventDefault: sandbox.stub().never(), + stopPropagation: sandbox.stub().never(), + });
422-426: Patch the ArrowLeft test’s preventDefault mock the same way.I pity the fool who repeats the same sinon misfire—
sandbox.mock().never()ain’t a callable. Use a stub (or spy) so the DOM event path can invoke it without exploding.- wrapper.find('.bdl-PillSelector').simulate('keyDown', { - key: 'ArrowLeft', - preventDefault: sandbox.mock().never(), - stopPropagation: sandbox.mock().never(), - }); + wrapper.find('.bdl-PillSelector').simulate('keyDown', { + key: 'ArrowLeft', + preventDefault: sandbox.stub().never(), + stopPropagation: sandbox.stub().never(), + });
477-481: Last one—swap those mocks for stubs, sucka.I pity the fool who leaves
sandbox.mock().never()here; ArrowRight will crash the test the same way. Make them stubs so the handlers can be invoked safely.- wrapper.find('.bdl-PillSelector').simulate('keyDown', { - key: 'ArrowRight', - preventDefault: sandbox.mock().never(), - stopPropagation: sandbox.mock().never(), - }); + wrapper.find('.bdl-PillSelector').simulate('keyDown', { + key: 'ArrowRight', + preventDefault: sandbox.stub().never(), + stopPropagation: sandbox.stub().never(), + });src/components/context-menu/__tests__/ContextMenu.test.tsx (1)
273-284: Test logic inverted: simulating “became disabled” incorrectly.You pass prevProps.isDisabled = true while current props remain false, but the test reads “becomes disabled.” Trigger the actual transition (false → true) so componentDidUpdate conditions are correct.
- wrapper.setState({ isOpen: true }); - const instance = wrapper.instance() as ContextMenu; - sandbox.mock(instance).expects('handleMenuClose'); - instance.componentDidUpdate({ isDisabled: true } as ContextMenuProps, { isOpen: true } as ContextMenuState); + wrapper.setState({ isOpen: true }); + const instance = wrapper.instance() as ContextMenu; + sandbox.mock(instance).expects('handleMenuClose'); + // Transition to disabled; this should invoke componentDidUpdate and close the menu + wrapper.setProps({ isDisabled: true });src/components/flyout/__tests__/Flyout.test.js (1)
395-409: Fix the hover-delay test: wrong prop and toggle, or your test’s gonna cryYou’re passing
openOnHoverDebounceTimeout(nonexistent) andopenOnHover: false. The code usesopenOnHoverDelayTimeoutand only opens whenopenOnHoveris true.Apply this diff:
- const timeout = 100; - const wrapper = renderWrapper({ openOnHover: false, openOnHoverDebounceTimeout: timeout }); + const timeout = 100; + const wrapper = renderWrapper({ openOnHover: true, openOnHoverDelayTimeout: timeout });Optional: use fake timers to avoid flakiness.
describe('handleButtonHover()', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + afterEach(() => { + jest.useRealTimers(); + });Then replace setTimeout assertions with
jest.advanceTimersByTime(...).
🧹 Nitpick comments (27)
src/components/context-menu/__tests__/ContextMenu.test.tsx (5)
56-69: Mount is right. Clean up or you’ll leak, fool.Good switch to mount. But unmount mounted wrappers in these tests to avoid leaking DOM/state across specs. Add wrapper.unmount() at the end (or a local afterEach for this describe).
113-119: Find Tether by component type, not prop sniffing.Prop-sniffing is brittle. Import TetherComponent and target it directly.
Apply this diff here:
- // Find the TetherComponent by looking for one with renderTarget prop - const tetherComponent = wrapper.findWhere(node => node.prop('renderTarget') && node.prop('renderElement')); + // Find Tether by component type for stability + const tetherComponent = wrapper.find(TetherComponent);And add the import near the top:
import TetherComponent from 'react-tether';Based on learnings
161-163: Don’t mock setState; assert observable effects instead.Mocking React internals is brittle. Prefer calling closeMenu() and asserting wrapper.state('isOpen') is false (and any side-effects) rather than expecting a specific setState shape.
252-255: Use wrapper.setState(), not instance.setState().Calling instance.setState() bypasses Enzyme bookkeeping. Use wrapper.setState({ isOpen: true }) and wrapper.update() for consistency.
- act(() => { - instance.setState({ isOpen: true }); - }); + act(() => { + wrapper.setState({ isOpen: true }); + }); + wrapper.update();
412-417: Streamline MouseEvent typing in tests.Double-casting to unknown is noisy. Extract a tiny helper to create typed-but-partial MouseEvents for clarity.
Example helper (place near the top of the file):
const makeMouseEvent = (overrides: Partial<MouseEvent> = {}) => overrides as unknown as MouseEvent;Then:
const handleContextMenuEvent = makeMouseEvent({ clientX: 10, clientY: 15, preventDefault: preventDefaultSpy }); const documentClickEvent = makeMouseEvent({ target: document.createElement('div') });Also applies to: 421-424, 439-444, 448-451
src/components/text-area/__tests__/TextArea.test.js (1)
75-79: Target TetherComponent directly to avoid brittle findWhere.Be explicit to reduce false positives, fool.
Apply this diff:
- const wrapper = mount(<TextArea error="error" label="label" tooltipTetherClassName={className} />); - const tetherEl = wrapper.findWhere(node => node.prop('renderTarget') && node.prop('renderElement')); + const wrapper = mount(<TextArea error="error" label="label" tooltipTetherClassName={className} />); + const tetherEl = wrapper.find('TetherComponent').first();src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
201-238: Tidy duplicate condition; consider rendering to body to avoid clipping.You check
isConfirmingDeletetwice; simplify. Also considerrenderElementTo={document.body}so the confirm modal ain’t clipped by overflow parents, fool.Apply this diff:
<TetherComponent {...tetherProps} + renderElementTo={document.body} renderTarget={ref => ( <div ref={ref} style={{ display: 'inline-block' }}> {isMenuVisible && ( <AnnotationActivityMenu @@ - renderElement={ref => { - return isConfirmingDelete ? ( - <div ref={ref}> - {isConfirmingDelete && ( - <DeleteConfirmation - data-resin-component={ACTIVITY_TARGETS.ANNOTATION_OPTIONS} - isOpen={isConfirmingDelete} - message={messages.annotationActivityDeletePrompt} - onDeleteCancel={handleDeleteCancel} - onDeleteConfirm={handleDeleteConfirm} - /> - )} - </div> - ) : null; - }} + renderElement={ref => + isConfirmingDelete ? ( + <div ref={ref}> + <DeleteConfirmation + data-resin-component={ACTIVITY_TARGETS.ANNOTATION_OPTIONS} + isOpen={isConfirmingDelete} + message={messages.annotationActivityDeletePrompt} + onDeleteCancel={handleDeleteCancel} + onDeleteConfirm={handleDeleteConfirm} + /> + </div> + ) : null + } />src/components/button-group/ButtonGroup.scss (1)
95-120: Disabled state misses nested buttons — cover the wrapper too, suckaWhen a wrapper is inserted,
.btninside won’t get disabled styles. Mirror the nested selectors here.Apply this diff:
&.bdl-is-disabled, &.is-disabled { &, & > .bdl-targeted-click-through { - > .btn, - > .bdl-Button { + > .btn, + > .bdl-Button, + > * > .btn, + > * > .bdl-Button { color: $bdl-gray-65; background-color: $bdl-gray-02; border: 1px solid $bdl-gray-30; box-shadow: none; cursor: default; opacity: 0.4; }src/components/flyout/Flyout.js (1)
494-496: Add a wrapper class for easier styling hooksGive the target wrapper a class so shared styles (e.g., targeted click-through) can key off it.
Apply this diff:
- <div ref={ref} style={{ display: 'inline-block' }}> + <div ref={ref} className="bdl-targeted-click-through" style={{ display: 'inline-block' }}> {React.cloneElement(overlayButton, overlayButtonProps)} </div>src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (1)
49-49: Clean up mounted wrappers to avoid stray portaled nodesMount leaves tethered elements attached; add unmounts to keep JSDOM clean and prevent cross-test bleed.
Example pattern:
describe('…', () => { + afterEach(() => { + // Unmount any wrappers created in tests + // If wrappers are local, call wrapper.unmount() at the end of each test + });I pity the fool who leaves portals hanging.
src/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.js (1)
2-2: Unify selectors across tests and unmount wrappersYou’re mixing wrapper.exists(Media.Menu) (shallow) and 'button.bdl-Media-menu' (mount). Prefer one approach for clarity, and unmount mounted wrappers to prevent tether leftovers.
I pity the fool who chases flaky tests due to portal residue.
Also applies to: 115-129
src/components/radar/__tests__/RadarAnimation.test.tsx (1)
6-12: Tidy up: unmount wrappers after mountMounted tethered components can leave portaled nodes. Add afterEach(() => wrapper.unmount()) or unmount at test end.
I pity the fool who lets portals pile up.
src/features/unified-share-modal/SharedLinkSection.js (1)
429-431: Consider adding a target wrapper class to the settings tooltip for consistencyFor consistent anchoring/styling with the toggle, add a target wrapper class to the settings tooltip too.
Example:
<Tooltip targetWrapperClassName="usm-ftux-toggle-tooltip-target" …>I pity the fool who leaves inconsistent targets.
src/elements/content-sidebar/activity-feed/task-new/Task.js (2)
296-327: Prefer a CSS class over inline style on the target wrapperReplace style={{ display: 'inline-block' }} with a class to keep styling in CSS.
- <div ref={ref} style={{ display: 'inline-block' }}> + <div ref={ref} className="bcs-Task-menuTarget">And in Task.scss:
.bcs-Task-menuTarget { display: inline-block; }I pity the fool who mixes styles in JS.
124-130: Optional: close confirmation immediately on confirmIf deletion is async and the card lingers, consider setting isConfirmingDelete=false in handleDeleteConfirm for a snappier UX (mirrors AnnotationActivity).
handleDeleteConfirm = (): void => { const { id, onDelete, permissions } = this.props; - if (onDelete) { + this.setState({ isConfirmingDelete: false }); + if (onDelete) { onDelete({ id, permissions }); } };I pity the fool who leaves modals hangin’.
Also applies to: 332-337
src/features/left-sidebar/LeftSidebarLinkCallout.js (1)
73-77: Ditch the inline style and use a CSS class or wrapper prop, fool.Inline-block via style works but isn’t consistent with the new target wrapper pattern elsewhere (Tooltip, Dropdown, Radar). Prefer a class so styling’s centralized.
Example:
- <div ref={ref} style={{ display: 'inline-block' }}> + <div ref={ref} className="bdl-LeftSidebarLinkCallout-target">Optionally add a targetWrapperClassName prop for parity with other components. Based on learnings
src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js (1)
66-83: ClassName propagation test looks fine; consider cleanup, fool.Mount without unmount can leak in JSDOM-heavy suites. Optional: unmount in afterEach or here.
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (1)
33-35: Tighten Tether selection to avoid false positivesPrefer wrapper.find(TetherComponent) or a test id on the Tether root over a predicate that just checks for render props. Be precise, fool.
-const findTetherComponent = wrapper => { - return wrapper.findWhere(node => node.prop('renderTarget') && node.prop('renderElement')); -}; +const findTetherComponent = wrapper => wrapper.find('TetherComponent');src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (2)
57-66: Fix ref param types — react-tether passes callback refs, foolType the ref params as React.Ref to match react-tether v3. Don’t lie to TypeScript, sucka.
- renderTarget={ref => ( + renderTarget={(ref: React.Ref<HTMLDivElement>) => ( <div ref={ref} style={{ display: 'inline-block' }}> <Media.Menu- renderElement={ref => { + renderElement={(ref: React.Ref<HTMLDivElement>) => { return isConfirmingDelete ? ( <div ref={ref}> <DeleteConfirmationAlso applies to: 116-128
71-91: Separate resin targets for resolve vs unresolve, foolBoth actions use COMMENT_OPTIONS_EDIT, muddying analytics. Give them distinct targets so you know who did what.
- <MenuItem + <MenuItem className="bcs-Comment-unresolveComment" - data-resin-target={ACTIVITY_TARGETS.COMMENT_OPTIONS_EDIT} + data-resin-target={ACTIVITY_TARGETS.COMMENT_OPTIONS_UNRESOLVE}- <MenuItem + <MenuItem - data-resin-target={ACTIVITY_TARGETS.COMMENT_OPTIONS_EDIT} + data-resin-target={ACTIVITY_TARGETS.COMMENT_OPTIONS_RESOLVE}Follow-up: add COMMENT_OPTIONS_RESOLVE and COMMENT_OPTIONS_UNRESOLVE to ACTIVITY_TARGETS before applying this. I pity the fool who ships fuzzy metrics.
src/components/radar/RadarAnimation.tsx (1)
132-134: Don’t mount hidden radar — gate DOM on isShown and add aria-hiddenKeeping invisible DOM hurts a11y and hit-testing. Render it only when shown and mark as aria-hidden for assistive tech, fool.
- const referenceElement = React.cloneElement(child as React.ReactElement, { - 'aria-describedby': this.radarAnimationID, - }); + const referenceElement = React.cloneElement(child as React.ReactElement, { + ...(isShown ? { 'aria-describedby': this.radarAnimationID } : {}), + });- renderElement={ref => ( - <div ref={ref} className={`radar ${className}`} id={this.radarAnimationID} {...rest}> - <div className="radar-dot" /> - <div className="radar-circle" /> - </div> - )} + renderElement={ref => + isShown ? ( + <div + ref={ref} + className={`radar ${className}`} + id={this.radarAnimationID} + aria-hidden="true" + {...rest} + > + <div className="radar-dot" /> + <div className="radar-circle" /> + </div> + ) : null + }Also applies to: 167-173
src/elements/content-sidebar/activity-feed/comment/Comment.js (1)
189-245: Wrap target in inline-block for consistent tether anchoringMatch other components and give the target wrapper style={{ display: 'inline-block' }}. Don’t let layout shifts punk your positioning.
- renderTarget={ref => ( - <div ref={ref}> + renderTarget={ref => ( + <div ref={ref} style={{ display: 'inline-block' }}> <Media.Menusrc/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js (3)
122-147: Tidy up mounted wrappers after each caseNow that the menu portals to
document.body, leaving the wrapper mounted can leak DOM between parameterized runs. Drop an explicit unmount so each iteration starts clean, fool.expect(wrapper.find('[data-testid="comment-actions-menu"]').exists()).toBe(showMenu); + + wrapper.unmount();Hit the other new
mountblocks the same way so no stray overlays hang around.
167-185: Same unmount cleanup applies hereThis loop mounts a menu that now portals to the body, too. I pity the fool who leaves it dangling—add an unmount before the test exits just like the earlier case.
205-223: Unresolve tests need the teardown as wellOne more mount, one more teardown. Pop in
wrapper.unmount()so every run leaves the DOM spotless, sucker.src/components/media/Media.scss (1)
8-10: Harden selector to cover both default and custom wrapper classesIf DropdownMenu doesn’t always include bdl-DropdownMenu-target, your new MediaMenu override may miss. Add the custom class too, fool, so display: block applies either way.
- .bdl-DropdownMenu-target { - display: block; - } + .bdl-DropdownMenu-target, + .bdl-Media-menu-target { + display: block; + }src/components/media/MediaMenu.tsx (1)
41-46: Prop precedence – reorder spread if defaults must win
You spread {...dropdownProps} after explicit props, letting callers override constrainToScrollParent, isRightAligned and targetWrapperClassName; move the spread before your defaults if you want your values to take precedence. DropdownMenu always prepends its own'bdl-DropdownMenu-target', so your.bdl-Media-menu-targetrule will always apply. I pity the fool who ignores this!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
src/components/footer-indicator/__tests__/__snapshots__/FooterIndicator.test.tsx.snapis excluded by!**/*.snapsrc/components/media/__tests__/__snapshots__/Media.test.tsx.snapis excluded by!**/*.snapsrc/components/menu/__tests__/__snapshots__/MenuItem.test.tsx.snapis excluded by!**/*.snapsrc/components/radar/__tests__/__snapshots__/RadarAnimation.test.tsx.snapis excluded by!**/*.snapsrc/components/select/__tests__/__snapshots__/Select.test.js.snapis excluded by!**/*.snapsrc/components/sidebar-toggle-button/__tests__/__snapshots__/SidebarToggleButton.test.js.snapis excluded by!**/*.snapsrc/components/tooltip/__tests__/__snapshots__/Tooltip.test.tsx.snapis excluded by!**/*.snapsrc/elements/content-sidebar/activity-feed/app-activity/__tests__/__snapshots__/AppActivity.test.js.snapis excluded by!**/*.snapsrc/elements/content-sidebar/activity-feed/comment/__tests__/__snapshots__/Comment.test.js.snapis excluded by!**/*.snapsrc/elements/content-sidebar/activity-feed/task-new/__tests__/__snapshots__/Task.test.js.snapis excluded by!**/*.snapsrc/features/collapsible-sidebar/__tests__/__snapshots__/CollapsibleSidebarMenuItem.test.js.snapis excluded by!**/*.snapsrc/features/left-sidebar/__tests__/__snapshots__/LeftSidebarLinkCallout.test.js.snapis excluded by!**/*.snapsrc/features/pagination/__tests__/__snapshots__/MarkerBasedPagination.test.js.snapis excluded by!**/*.snapsrc/features/pagination/__tests__/__snapshots__/OffsetBasedPagination.test.js.snapis excluded by!**/*.snapsrc/features/sandbox-banner/__tests__/__snapshots__/SandboxBanner.test.tsx.snapis excluded by!**/*.snapsrc/features/unified-share-modal/__tests__/__snapshots__/SharedLinkSection.test.js.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (48)
.flowconfig(1 hunks)package.json(2 hunks)src/components/button-group/ButtonGroup.scss(3 hunks)src/components/checkbox/Checkbox.scss(1 hunks)src/components/context-menu/ContextMenu.tsx(1 hunks)src/components/context-menu/__tests__/ContextMenu.test.tsx(9 hunks)src/components/draft-js-editor/DraftJSEditor.js(1 hunks)src/components/draft-js-editor/DraftJSEditor.scss(1 hunks)src/components/dropdown-menu/DropdownMenu.js(3 hunks)src/components/dropdown-menu/DropdownMenu.scss(1 hunks)src/components/dropdown-menu/__tests__/DropdownMenu.test.js(17 hunks)src/components/flyout/Flyout.js(1 hunks)src/components/flyout/__tests__/Flyout.test.js(20 hunks)src/components/footer-indicator/FooterIndicator.scss(1 hunks)src/components/footer-indicator/FooterIndicator.tsx(1 hunks)src/components/media/Media.scss(1 hunks)src/components/media/MediaMenu.tsx(1 hunks)src/components/menu/MenuItem.scss(1 hunks)src/components/menu/MenuItem.tsx(2 hunks)src/components/pill-selector-dropdown/__tests__/PillSelector.test.js(16 hunks)src/components/radar/RadarAnimation.scss(1 hunks)src/components/radar/RadarAnimation.stories.tsx(1 hunks)src/components/radar/RadarAnimation.tsx(5 hunks)src/components/radar/__tests__/RadarAnimation.test.tsx(2 hunks)src/components/select/Select.js(1 hunks)src/components/text-area/__tests__/TextArea.test.js(2 hunks)src/components/text-input/__tests__/TextInput.test.js(1 hunks)src/components/tooltip/Tooltip.scss(1 hunks)src/components/tooltip/Tooltip.tsx(5 hunks)src/components/tooltip/__tests__/Tooltip.test.tsx(14 hunks)src/elements/content-sidebar/SidebarNavButton.js(3 hunks)src/elements/content-sidebar/SidebarNavButton.scss(4 hunks)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js(1 hunks)src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js(10 hunks)src/elements/content-sidebar/activity-feed/app-activity/AppActivity.js(1 hunks)src/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.js(2 hunks)src/elements/content-sidebar/activity-feed/comment/Comment.js(2 hunks)src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js(6 hunks)src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx(1 hunks)src/elements/content-sidebar/activity-feed/task-new/Task.js(1 hunks)src/elements/content-sidebar/activity-feed/task-new/__tests__/Task.test.js(3 hunks)src/features/collapsible-sidebar/CollapsibleSidebar.scss(1 hunks)src/features/collapsible-sidebar/CollapsibleSidebarMenuItem.js(1 hunks)src/features/left-sidebar/LeftSidebarLinkCallout.js(1 hunks)src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js(3 hunks)src/features/sandbox-banner/SandboxBanner.tsx(1 hunks)src/features/unified-share-modal/SharedLinkSection.js(3 hunks)src/features/unified-share-modal/UnifiedShareModal.scss(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/features/unified-share-modal/UnifiedShareModal.scss
- src/components/footer-indicator/FooterIndicator.tsx
- src/components/dropdown-menu/DropdownMenu.scss
- .flowconfig
- src/elements/content-sidebar/activity-feed/app-activity/AppActivity.js
- src/features/collapsible-sidebar/CollapsibleSidebar.scss
- src/components/text-input/tests/TextInput.test.js
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-09-09T21:43:34.180Z
Learnt from: bxie-box
PR: box/box-ui-elements#4271
File: src/features/classification/README.md:67-67
Timestamp: 2025-09-09T21:43:34.180Z
Learning: In the Classification component's aiClassificationReason prop, TooltipProvider is only required when the citations array is empty (shows tooltip UI), not when citations are present (shows card on hover UI).
Applied to files:
src/components/draft-js-editor/DraftJSEditor.js
📚 Learning: 2025-09-16T14:53:03.538Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4275
File: src/components/form-elements/draft-js-mention-selector/index.js:4-4
Timestamp: 2025-09-16T14:53:03.538Z
Learning: The createMentionSelectorState export in src/components/form-elements/draft-js-mention-selector/index.js was confirmed to be internal-only within the box-ui-elements package, with no external references or documentation, making the breaking rename to createMentionTimestampSelectorState safe.
Applied to files:
src/components/draft-js-editor/DraftJSEditor.jssrc/components/select/Select.js
📚 Learning: 2025-09-03T18:10:42.760Z
Learnt from: fpan225
PR: box/box-ui-elements#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/SidebarNavButton.scsssrc/elements/content-sidebar/SidebarNavButton.js
📚 Learning: 2025-08-19T17:50:47.187Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4228
File: src/elements/content-sidebar/activity-feed/comment/Comment.js:147-148
Timestamp: 2025-08-19T17:50:47.187Z
Learning: In the box-ui-elements codebase, ActivityThreadReplyForm.js and AnnotationThreadCreate.js component files exist but are not actively used/imported anywhere, so they don't need updates for prop propagation.
Applied to files:
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.jssrc/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js
📚 Learning: 2025-09-15T17:04:28.279Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4275
File: src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js:259-260
Timestamp: 2025-09-15T17:04:28.279Z
Learning: When wrapping enzyme mount() tests with IntlProvider for class-based components, use wrapper.find('ComponentName') to access the actual component instance and state, since the wrapper becomes the provider instead of the component.
Applied to files:
src/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.jssrc/components/context-menu/__tests__/ContextMenu.test.tsxsrc/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.jssrc/components/tooltip/__tests__/Tooltip.test.tsxsrc/components/text-area/__tests__/TextArea.test.jssrc/components/radar/__tests__/RadarAnimation.test.tsxsrc/components/flyout/__tests__/Flyout.test.js
📚 Learning: 2025-09-03T18:10:29.467Z
Learnt from: fpan225
PR: box/box-ui-elements#4239
File: src/elements/content-sidebar/SidebarNav.js:94-106
Timestamp: 2025-09-03T18:10:29.467Z
Learning: In the ContentSidebar CustomSidebarPanel API, the navButtonProps spread should come after explicit data-* attributes to allow users to override analytics tracking attributes like data-resin-target, data-target-id, and data-testid when needed. This is intentional API design for flexibility.
Applied to files:
src/elements/content-sidebar/SidebarNavButton.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sidebar/SidebarNavButton.js
📚 Learning: 2025-08-27T17:03:48.322Z
Learnt from: bxie-box
PR: box/box-ui-elements#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/components/tooltip/__tests__/Tooltip.test.tsx
🧬 Code graph analysis (17)
src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (3)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (6)
handleMenuClose(99-99)isConfirmingDelete(63-63)isResolved(84-84)canDelete(79-79)handleDeleteCancel(87-87)handleDeleteConfirm(88-91)src/elements/content-sidebar/activity-feed/comment/Comment.js (4)
canResolve(165-165)isResolved(170-170)canEdit(164-164)canDelete(163-163)src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenuWrapper.js (3)
handleStatusUpdate(69-73)handleEditClick(56-60)handleDeleteClick(51-54)
src/components/context-menu/ContextMenu.tsx (2)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (1)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
AnnotationActivity(48-98)
src/components/flyout/Flyout.js (1)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (1)
src/components/flyout/__tests__/Flyout.test.js (4)
renderWrapper(39-46)FakeButton(19-24)findTetherComponent(48-50)sandbox(7-7)
src/components/context-menu/__tests__/ContextMenu.test.tsx (1)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (3)
FakeButton(12-12)FakeMenu(16-20)sandbox(8-8)
src/elements/content-sidebar/activity-feed/task-new/Task.js (2)
src/elements/common/interactionTargets.js (2)
ACTIVITY_TARGETS(34-59)ACTIVITY_TARGETS(34-59)src/elements/content-sidebar/activity-feed/task-new/messages.js (1)
messages(9-188)
src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js (2)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (1)
findTetherComponent(33-35)src/components/flyout/__tests__/Flyout.test.js (1)
findTetherComponent(48-50)
src/elements/content-sidebar/SidebarNavButton.js (2)
src/elements/content-sidebar/__tests__/withSidebarAnnotations.rtl.test.js (1)
internalSidebarNavigationHandler(55-55)src/utils/dom.js (1)
isLeftClick(79-81)
src/features/unified-share-modal/SharedLinkSection.js (2)
src/elements/content-sharing/SharingModal.js (2)
sharedLink(75-75)sharedLink(228-234)src/features/unified-share-modal/messages.js (1)
messages(3-581)
src/components/text-area/__tests__/TextArea.test.js (1)
src/components/text-area/TextArea.js (1)
TextArea(33-91)
src/elements/content-sidebar/activity-feed/comment/Comment.js (3)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (3)
isConfirmingDelete(63-63)isResolved(84-84)canDelete(79-79)src/elements/content-sidebar/activity-feed/comment/BaseComment.js (4)
canResolve(143-143)isResolved(147-147)canEdit(142-142)canDelete(141-141)src/elements/content-sidebar/activity-feed/comment/messages.js (1)
messages(9-65)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (3)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/common/interactionTargets.js (2)
ACTIVITY_TARGETS(34-59)ACTIVITY_TARGETS(34-59)src/elements/content-sidebar/activity-feed/annotations/messages.js (1)
messages(3-50)
src/components/tooltip/Tooltip.tsx (2)
src/components/flyout/Flyout.js (2)
tetherProps(454-463)tetherPosition(392-392)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/components/radar/RadarAnimation.tsx (3)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)src/elements/content-open-with/ContentOpenWith.js (1)
className(549-549)
src/components/radar/__tests__/RadarAnimation.test.tsx (3)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (2)
renderWrapper(24-31)findTetherComponent(33-35)src/components/flyout/__tests__/Flyout.test.js (2)
renderWrapper(39-46)findTetherComponent(48-50)src/components/radar/RadarAnimation.tsx (1)
RadarAnimationProps(61-80)
src/components/flyout/__tests__/Flyout.test.js (1)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (4)
renderWrapper(24-31)FakeButton(12-12)findTetherComponent(33-35)sandbox(8-8)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarNavButton.js
[error] 14-18: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (53)
src/components/context-menu/__tests__/ContextMenu.test.tsx (3)
72-81: LGTM on closed-menu rendering.Mount-based assertion is clear and accurate.
84-94: Nice use of act() + update().This will keep state/render in sync. I pity the flake that don’t!
297-299: LGTM on unmount behavior assertions.Sinon expectations read clean; good guard that no listeners are removed when closed.
src/components/menu/MenuItem.scss (1)
1-3: Block display OK, but verify no layout shift on MenuItem.Looks fine, sucka. I pity the fool who doesn’t check this doesn’t widen inline targets unexpectedly in dense menus.
src/components/footer-indicator/FooterIndicator.scss (1)
39-42: LGTM — consistent tooltip target hook.Solid addition. I pity the fool who forgets to ensure parent height is defined (it is here).
src/components/radar/RadarAnimation.scss (1)
121-123: LGTM — inline-block target matches usage.Keeps the anchor box tight. Clean.
src/components/radar/RadarAnimation.stories.tsx (1)
61-65: LGTM — local margin wrapper for story layout.Looks good, sucka.
src/elements/content-sidebar/SidebarNavButton.scss (2)
27-27: LGTM — attribute selector quote style change is harmless.No impact. Keep rolling.
62-64: New target hook class added — ensure it’s wired in component.I pity the fool who adds a class without using it. Confirm SidebarNavButton renders this via target wrapper/prop.
src/components/checkbox/Checkbox.scss (1)
79-82: Verify line-height hack across browsers.
line-height: 0.1should squash the line box, but test in Chrome/Firefox/Safari+Windows to avoid odd baseline shifts, fool.src/components/context-menu/ContextMenu.tsx (1)
171-184: Guarddocument.bodybefore render, fool!I pity the fool who lets this run on the server—
documentain’t there, so this blows up in SSR. Put a browser check in front of the lookup and hand Tether an optional target instead.- return ( - <TetherComponent - {...tetherProps} - renderElementTo={document.body} + const renderElementTarget = typeof document !== 'undefined' ? document.body : undefined; + + return ( + <TetherComponent + {...tetherProps} + renderElementTo={renderElementTarget}src/elements/content-sidebar/SidebarNavButton.js (3)
14-18: Flow type imports are legit — don’t let Biome hustle you, foolThis file is Flow (@flow). The
import typeis correct. If Biome squawks, tune its config to skip Flow parsing for these files.
59-65: Pure formatting, no logic change — good hustleBehavior unchanged. Ship it.
85-91: Consistent tooltip target wrapper in both paths — that’s tightAdding
targetWrapperClassName="bcs-NavButton-target"in both routerDisabled and router paths keeps layout solid. I pity the fool who ships asymmetry.Also applies to: 137-142
src/components/button-group/ButtonGroup.scss (1)
46-47: Selector expansion covers wrapped buttons — nice coverageThe
> * > .btnadditions handle new wrapper layers from tether targets. Good foresight.Also applies to: 68-75, 80-90
src/components/flyout/Flyout.js (1)
491-507: Clean migration to renderTarget/renderElement — that’s how you rollOnly renders overlay when visible and keeps tether props intact. Looks solid.
src/components/flyout/__tests__/Flyout.test.js (1)
39-51: Helper extraction (renderWrapper/findTetherComponent) — tight and tidyCentralized mounting and targeted locating of Tether keeps tests lean. Respect.
src/elements/content-sidebar/activity-feed/task-new/__tests__/Task.test.js (1)
301-314: Action menu assertions after real click — that’s the right kind of painSwitching to mount and asserting
MenuItemvisibility per permissions looks correct.Also applies to: 317-330
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (4)
2-2: Mount migration LGTM — solid moveSwitching to mount and centralizing it in getWrapper fits the react-tether v3 renderTarget/renderElement flow. I pity the fool who keeps shallow for tethered UIs.
Also applies to: 49-49
112-116: Correct use of React.act around stateful handlersWrapping onEdit/onCancel and calling wrapper.update() is right for state transitions. Keep it consistent across similar handler paths.
Also applies to: 119-123
241-246: Delete flow interactions verified — niceOpening, canceling, and confirming delete via props on the tethered components is exercised correctly with act + update. Looks good, fool.
Also applies to: 256-264, 278-286
338-342: Card disable on menu open — behavior alignedTying isDisabled to the menu open state matches the UX and prevents stray selections. Rock solid.
src/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.js (2)
76-88: Overflow menu presence check via mounted button — goodAsserting on the rendered menu button class after mount matches the tethered implementation. Nice upgrade.
90-102: Permissions-based visibility test — correctVerifies the alternate path (different user + can_delete) properly. Looks tight.
src/components/radar/__tests__/RadarAnimation.test.tsx (5)
6-16: Helper + tether finder look goodCentralized mount helper and findWhere(renderTarget/renderElement) is a clean way to target TetherComponent. Approved.
60-65: Snapshot on shallow for position variants is acceptableLightweight snapshot coverage across positions is fine here. Keep the matrix tight.
70-74: Offset prop assertion via tether component — correctReading offset from the tether node directly avoids implementation leakage. Solid.
Also applies to: 76-84
96-113: isShown toggles enabled prop — correctEnabled reflects isShown properly. Nice.
120-127: Imperative position() behavior — verifiedOnly positions when shown. Good guard; no wasted work. I like that.
src/features/unified-share-modal/SharedLinkSection.js (2)
530-537: Adding targetWrapperClassName to the toggle tooltip — nice touchWrapper class enables stable styling/targeting with the new tether pattern. Approved.
203-205: sharedLinkType includes all flags – Flow type declarescanChangeAccessLevel,isEditSettingAvailable,isDownloadSettingAvailable, andpermissionLevel; no extra runtime checks needed. I pity the fool.src/elements/content-sidebar/activity-feed/task-new/Task.js (1)
291-341: Tether refactor is correct — renderTarget/renderElement done rightInline-block target wrapper, disabled menu while confirming, and conditional DeleteConfirmation rendering look solid. Also, the missing return in renderElement is fixed — good catch and fix.
src/components/tooltip/__tests__/Tooltip.test.tsx (1)
525-527: Loosen that addEventListener check, fool!This exact brittle assertion already got called out. I pity the fool who keeps depending on a specific options signature—filter the spy’s calls for
'keydown'and assert the list is empty, then restore the spy.- expect(addEventListenerSpy).not.toHaveBeenCalledWith('keydown', expect.anything(), true); + const keydownCalls = addEventListenerSpy.mock.calls.filter(call => call[0] === 'keydown'); + expect(keydownCalls).toHaveLength(0);src/features/left-sidebar/LeftSidebarLinkCallout.js (2)
78-88: LGTM on the react-tether v3 migration, fool.Clean switch to renderTarget/renderElement. Conditional render looks tight.
75-75: Ensure only a single child is passed, or you’ll eat an exception.React.Children.only throws on arrays/fragments. Tighten the Flow type to React.Element or document the constraint. I pity the fool who passes multiple kids.
src/components/menu/MenuItem.tsx (2)
7-8: Import addition looks good, fool.SCSS hook in place for radar target styling.
73-73: Verify RadarAnimation supports targetWrapperClassName and wrapper display.Make sure:
- RadarAnimation indeed forwards targetWrapperClassName to a real wrapper.
- .bdl-MenuItem-radarTarget yields inline-block or otherwise preserves layout of inline menu items.
I pity the fool who breaks alignment.
src/components/draft-js-editor/DraftJSEditor.js (1)
129-135: Tooltip wrapper class addition is solid, fool.Matches the repo-wide target wrapper pattern. Ensure .bdl-DraftJSEditor-target styles exist (e.g., display block/inline-block as needed).
src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js (1)
7-9: Helper for finding TetherComponent is on point, fool.Consistent with other tests in the suite.
src/components/dropdown-menu/DropdownMenu.js (3)
16-18: Prop doc and addition look good, fool.targetWrapperClassName aligns with the cross-repo pattern.
187-187: Destructuring the new prop: all good, fool.
281-287: LGTM on renderElement, fool.Clean conditional render with proper ref/clone.
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (2)
24-35: Solid migration to renderTarget/renderElement, foolMount helper and Tether finder look good and align with react-tether v3 patterns. I pity the fool who sticks to children-based APIs.
150-161: LGTM on renderElementTo assertionsFallback to document.body and honoring a provided element both look correct. I pity the fool who breaks overlay mounting.
Also applies to: 174-184
src/components/tooltip/Tooltip.tsx (2)
347-387: Use React.Ref for renderElement’s ref, not RefObject, foolreact-tether hands you a callback ref. Type it correctly so TS doesn’t bite you later.
- const renderTooltip = (ref: React.RefObject<HTMLDivElement>) => { + const renderTooltip = (ref: React.Ref<HTMLDivElement>) => {
393-401: Fix renderTarget ref type and enforce element childI pity the fool who treats strings/fragments as children while calling Children.only. Tighten types and ref signature.
- renderTarget={(ref: React.RefObject<HTMLDivElement>) => { + renderTarget={(ref: React.Ref<HTMLDivElement>) => { const child = React.Children.only(children); return child ? ( <div className={classNames('bdl-Tooltip-target', targetWrapperClassName)} ref={ref}> {React.cloneElement(child as React.ReactElement, componentProps)} </div> ) : null; }}Additionally update TooltipProps to require a single element:
- /** A React element to put the tooltip on */ - children: React.ReactChild; + /** A single React element to put the tooltip on */ + children: React.ReactElement;src/elements/content-sidebar/activity-feed/comment/Comment.js (1)
202-222: Use distinct resin targets for resolve vs unresolve, foolSame target for both actions muddies analytics. Split them like a champ.
- className="bcs-Comment-unresolveComment" - data-resin-target={ACTIVITY_TARGETS.COMMENT_OPTIONS_EDIT} + className="bcs-Comment-unresolveComment" + data-resin-target={ACTIVITY_TARGETS.COMMENT_OPTIONS_UNRESOLVE}- <MenuItem - data-resin-target={ACTIVITY_TARGETS.COMMENT_OPTIONS_EDIT} + <MenuItem + data-resin-target={ACTIVITY_TARGETS.COMMENT_OPTIONS_RESOLVE}Remember to add these constants to ACTIVITY_TARGETS first.
src/components/tooltip/Tooltip.scss (1)
228-230: Solid inline-block wrapperNice work giving the tether target its own inline-block footing, sucker. This keeps the anchor from collapsing when the tooltip shows up. I pity the fool who skips that.
src/components/select/Select.js (1)
66-74: Tooltip wrapper class aligns with the new APIHanding the wrapper class to
Tooltipkeeps the styling hook alive while letting the button stay lean. I pity the fool who forgets to move those classes over. Looks tight.src/features/collapsible-sidebar/CollapsibleSidebarMenuItem.js (1)
198-205: Consistent tooltip target stylingDropping the target wrapper class here matches the new tooltip contract so the sidebar item keeps its layout in line. I pity the fool who leaves that out. Good hustle.
package.json (1)
269-270: Dependency bumps line upDependencies and peers both call for
react-tether@^3.0.3, keeping the ecosystem honest. I pity the fool who forgets to sync those ranges. All squared away.Also applies to: 350-351
src/components/draft-js-editor/DraftJSEditor.scss (1)
20-22: Approve: CSS class is applied
I pity the fool who thinks it’s unused—DraftJSEditor.js (src/components/draft-js-editor/DraftJSEditor.js:130) passes targetWrapperClassName="bdl-DraftJSEditor-target" to Tooltip, so the CSS takes effect.src/features/sandbox-banner/SandboxBanner.tsx (1)
13-15: No style regressions detectedSelectors
.bdl-SandboxBannerand.bdl-SandboxBanner-textremain top-level and apply correctly to the migrated DOM. I pity the fool who disagrees.
BREAKING CHANGE: Components using react-tether have had a <div> wrapper added, and may break some layouts.
jfox-box
force-pushed
the
react-tether-upgrade-3.0.3
branch
from
7dccf6b to
4d234bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (1)
1-3: Fix thatactimport before it fixes you, fool!
actain’t exported from'react', so this grab gives youundefinedand everyact(...)call crashes the suite on load. Pull it from'react-dom/test-utils'so the tests stay on their feet. I pity the fool who ships broken tests!-import React, { act } from 'react'; -import { mount, shallow } from 'enzyme'; +import React from 'react'; +import { act } from 'react-dom/test-utils'; +import { mount, shallow } from 'enzyme';src/components/button-group/ButtonGroup.scss (1)
51-66: Border-right color breaks with wrappers — fix last-of-type usageWith wrappers, each button is last-of-type within its wrapper, so all .btn-primary get the final right border color. I pity the fool who ships busted borders! Target the group’s last child instead.
Apply this diff:
&.btn-primary { margin: 5px 0; border-right-color: darken($primary-color, 10%); - &:last-of-type { - border-right-color: $primary-color; - } } + + // Ensure only the last button in the group gets the final right border color, even with wrappers + > .btn.btn-primary:last-child, + > *:last-child > .btn.btn-primary { + border-right-color: $primary-color; + }src/components/flyout/__tests__/Flyout.test.js (4)
371-382: Flaky hover-open test — replace setTimeout with fake timers and a spy.I pity the fool who waits real time. Use jest.useFakeTimers and advance time deterministically.
- test('should call openOverlay() when props.openOnHover is true', () => { - const event = {}; - const wrapper = renderWrapper({ openOnHover: true }); - - const instance = wrapper.instance(); - setTimeout(() => { - sandbox.mock(instance).expects('openOverlay'); - }, 310); // default timeout is 300ms - - instance.handleButtonHover(event); - }); + test('should call openOverlay() when props.openOnHover is true', () => { + jest.useFakeTimers(); + const wrapper = renderWrapper({ openOnHover: true }); + const instance = wrapper.instance(); + const spy = sandbox.spy(instance, 'openOverlay'); + instance.handleButtonHover({}); + act(() => { + jest.advanceTimersByTime(310); // default timeout is 300ms + }); + expect(spy.calledOnce).toBe(true); + jest.useRealTimers(); + });
383-394: Flaky hover-open negative test — same timer fix.Advance timers and assert not called. Don’t get played by the clock.
- test('should not call openOverlay() when props.openOnHover is false', () => { - const event = {}; - const wrapper = renderWrapper({ openOnHover: false }); - - const instance = wrapper.instance(); - setTimeout(() => { - sandbox.mock(instance).expects('openOverlay').never(); - }, 310); // default timeout is 300ms - - instance.handleButtonHover(event); - }); + test('should not call openOverlay() when props.openOnHover is false', () => { + jest.useFakeTimers(); + const wrapper = renderWrapper({ openOnHover: false }); + const instance = wrapper.instance(); + const spy = sandbox.spy(instance, 'openOverlay'); + instance.handleButtonHover({}); + act(() => { + jest.advanceTimersByTime(310); + }); + expect(spy.called).toBe(false); + jest.useRealTimers(); + });
395-409: Custom debounce test is also timing‑flaky — switch to fake timers.Make the assertions precise around timeout ± delta.
- test('should be able to set custom timeouts for the openOnHover', () => { - const timeout = 100; - const wrapper = renderWrapper({ openOnHover: false, openOnHoverDebounceTimeout: timeout }); - - const instance = wrapper.instance(); - setTimeout(() => { - sandbox.mock(instance).expects('openOverlay').never(); - }, timeout - 10); - - setTimeout(() => { - sandbox.mock(instance).expects('openOverlay'); - }, timeout + 10); - - instance.handleButtonHover({}); - }); + test('should be able to set custom timeouts for the openOnHover', () => { + jest.useFakeTimers(); + const timeout = 100; + const wrapper = renderWrapper({ openOnHover: true, openOnHoverDebounceTimeout: timeout }); + const instance = wrapper.instance(); + const spy = sandbox.spy(instance, 'openOverlay'); + instance.handleButtonHover({}); + act(() => { + jest.advanceTimersByTime(timeout - 10); + }); + expect(spy.called).toBe(false); + act(() => { + jest.advanceTimersByTime(20); + }); + expect(spy.calledOnce).toBe(true); + jest.useRealTimers(); + });
414-423: Hover leave test uses real timers — switch to fake timers + spy.Deterministic and fast; no pity for flaky tests.
- test('should call closeOverlay', () => { - const wrapper = renderWrapper({ openOnHover: false }); - - const instance = wrapper.instance(); - - setTimeout(() => { - sandbox.mock(instance).expects('closeOverlay'); - }, 310); - - instance.handleButtonHoverLeave({}); - }); + test('should call closeOverlay', () => { + jest.useFakeTimers(); + const wrapper = renderWrapper({ openOnHover: false }); + const instance = wrapper.instance(); + const spy = sandbox.spy(instance, 'closeOverlay'); + instance.handleButtonHoverLeave({}); + act(() => { + jest.advanceTimersByTime(310); + }); + expect(spy.calledOnce).toBe(true); + jest.useRealTimers(); + });src/elements/content-sidebar/activity-feed/app-activity/AppActivity.js (1)
51-61: Typo in rel attribute — use noopener noreferrer.Don’t get tracked, fool. Fix rel value.
- rel="roreferrer noopener" + rel="noopener noreferrer"
🧹 Nitpick comments (15)
src/components/draft-js-editor/DraftJSEditor.scss (1)
20-22: Double-check the display mode for the tether targetListen up, fool! I pity the fool who sets this wrapper to
display: blockif the upgraded tether flow expectsinline-blocktargets—most of the new target wrappers follow that pattern so the measurement stays tight around the content. Please confirm this editor wrapper won’t stretch across the container and throw off tether positioning, or flip it toinline-blockto stay consistent.src/components/text-area/__tests__/TextArea.test.js (1)
75-79: Add unmount cleanup for Tether mount() testListen up, fool! React‑tether keeps DOM hooks alive after mount, and I pity the fool who lets those leak into other specs. Drop an explicit
wrapper.unmount()so this test cleans up after itself.const tetherEl = wrapper.findWhere(node => node.prop('renderTarget') && node.prop('renderElement')); expect(tetherEl.prop('className')).toBe(className); + + wrapper.unmount();src/components/media/MediaMenu.tsx (1)
41-46: Defaults first, consumer overrides last — solid move.Passing
constrainToScrollParent,isRightAligned, andtargetWrapperClassNamebefore{...dropdownProps}lets callers override when needed. Good alignment with the new pattern.If/when
DropdownMenugets TS types, consider typingdropdownPropsasPartial<DropdownMenuProps>instead of{}for safer usage.src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js (1)
122-134: Mounted tests and explicit menu open — solid. Consider DRY + unmountSwitch to mount and clicking the menu before assertions is right. To tighten it up:
- Add a small openMenu helper to avoid repetition.
- Unmount wrappers or use afterEach(() => wrapper?.unmount()) to prevent leaking mounted trees.
I pity the fool who ignores flaky tests.
Also applies to: 136-142, 144-147, 167-178, 180-184, 205-215, 217-221
src/features/sandbox-banner/SandboxBanner.tsx (1)
13-15: Adopting targetWrapperClassName — LGTMGood use of Tooltip’s wrapper class. Verify SandboxBanner.scss still targets .bdl-SandboxBanner correctly with the wrapper now provided by Tooltip. Optional: broaden children type to React.ReactNode for flexibility.
src/components/checkbox/Checkbox.scss (1)
78-82: Tooltip wrapper line-height hack — verify cross-browserline-height: 0.1 can cause baseline/cropping quirks, especially on Windows/Zoom. Consider relying on inline-flex + align-items: center; and explicit height instead, or at least QA this across browsers.
src/elements/content-sidebar/SidebarNavButton.js (1)
60-66: Make hasExtraParams boolean-safeCurrent truthiness can misread 0 as false. Coerce explicitly to avoid surprises.
Apply this diff:
- const hasExtraParams = - internalSidebarNavigation && - (internalSidebarNavigation.versionId || - internalSidebarNavigation.activeFeedEntryType || - internalSidebarNavigation.activeFeedEntryId || - internalSidebarNavigation.fileVersionId); + const hasExtraParams = Boolean( + internalSidebarNavigation && + (internalSidebarNavigation.versionId != null || + internalSidebarNavigation.activeFeedEntryType != null || + internalSidebarNavigation.activeFeedEntryId != null || + internalSidebarNavigation.fileVersionId != null), + );src/elements/content-sidebar/activity-feed/task-new/__tests__/Task.test.js (2)
301-314: Menu assertions look good; consider guarding for portal timing.Solid switch to mount and explicit MenuItem exists checks. I pity the fool who forgets portal timing — add wrapper.update() after the click to avoid flakiness if the menu opens async.
Apply this diff:
- wrapper.find('button[data-testid="task-actions-menu"]').simulate('click'); + wrapper.find('button[data-testid="task-actions-menu"]').simulate('click'); + wrapper.update();
317-330: Symmetric edit/delete checks are clear; same portal-timing note applies.Mirror the update after click here, too. Don’t let async menus make a fool outta your tests.
- wrapper.find('button[data-testid="task-actions-menu"]').simulate('click'); + wrapper.find('button[data-testid="task-actions-menu"]').simulate('click'); + wrapper.update();src/components/flyout/__tests__/Flyout.test.js (1)
834-853: Event listener add/remove expectations are brittle to options.Consider filtering by event type or using sinon.match for args to avoid failing on capture/passive flags. Don’t get fooled by extra params.
- documentMock.expects('addEventListener').withArgs('click'); - documentMock.expects('addEventListener').withArgs('contextmenu'); + documentMock.expects('addEventListener').withArgs('click', sinon.match.any, sinon.match.any); + documentMock.expects('addEventListener').withArgs('contextmenu', sinon.match.any, sinon.match.any);Similarly for removeEventListener.
src/elements/content-sidebar/activity-feed/app-activity/AppActivity.js (1)
150-171: Use react-tether v3 classes instead of className; add target wrapper class.In v3, styling the portaled element should go via classes={{ element: ... }}, not className. I pity the fool who loses styles in a portal. Also prefer a class over inline display style.
- <TetherComponent - attachment="top right" - className="bcs-AppActivity-confirm" - constraints={[{ to: 'scrollParent', attachment: 'together' }]} - targetAttachment="bottom right" + <TetherComponent + attachment="top right" + classes={{ element: 'bcs-AppActivity-confirm' }} + constraints={[{ to: 'scrollParent', attachment: 'together' }]} + targetAttachment="bottom right" renderTarget={ref => ( - <div ref={ref} style={{ display: 'inline-block' }}> + <div ref={ref} className="bdl-AppActivity-target"> <Media.Menu isDisabled={isConfirmingDelete}> <MenuItem onClick={this.handleDeleteClick}> <IconTrash color={bdlGray80} /> <FormattedMessage {...messages.appActivityDeleteMenuItem} /> </MenuItem> </Media.Menu> </div> )} renderElement={ref => { return isConfirmingDelete ? ( - <div ref={ref}> + <div ref={ref}> <DeleteConfirmationIf className still works in your local react-tether wrapper, confirm via a quick storybook check.
src/features/unified-share-modal/SharedLinkSection.js (1)
530-534: Be consistent with target wrapper to avoid layout jitterAdd the same targetWrapperClassName to the FTUX tooltip variant so the toggle’s wrapper class is consistent in both branches. I pity the fool who ships subtle layout shifts.
You can mirror the addition here:
- Above (when sharedLinkToggleTooltip is truthy), add:
<Tooltip className="usm-ftux-tooltip" targetWrapperClassName="usm-ftux-toggle-tooltip-target" ... >src/elements/content-sidebar/activity-feed/task-new/Task.js (1)
296-299: Drop inline style; use a shared inline-block classInline styles are brittle. Prefer a reusable class (e.g., bdl-Media-menu-target) to enforce inline-block and keep styles centralized.
Apply:
- renderTarget={ref => ( - <div ref={ref} style={{ display: 'inline-block' }}> + renderTarget={ref => ( + <div ref={ref} className="bdl-Media-menu-target">src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (1)
57-115: Replace inline style with a target wrapper classUse a CSS class for inline-block instead of style. Keeps consistency with other tether targets, fool.
- renderTarget={ref => ( - <div ref={ref} style={{ display: 'inline-block' }}> + renderTarget={ref => ( + <div ref={ref} className="bdl-Media-menu-target">src/components/radar/__tests__/RadarAnimation.test.tsx (1)
6-12: Drop the generic on mount; type the wrapper instead.Enzyme’s TS types don’t support mount cleanly. Avoid the generic to prevent TS friction.
- const renderWrapper = (props: Partial<RadarAnimationProps> & Record<string, unknown> = {}) => { - return mount<RadarAnimation>( + const renderWrapper = ( + props: Partial<RadarAnimationProps> & Record<string, unknown> = {}, + ): ReactWrapper => { + return mount( <RadarAnimation {...props}> <div>Hello</div> </RadarAnimation>, ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
src/components/footer-indicator/__tests__/__snapshots__/FooterIndicator.test.tsx.snapis excluded by!**/*.snapsrc/components/media/__tests__/__snapshots__/Media.test.tsx.snapis excluded by!**/*.snapsrc/components/menu/__tests__/__snapshots__/MenuItem.test.tsx.snapis excluded by!**/*.snapsrc/components/radar/__tests__/__snapshots__/RadarAnimation.test.tsx.snapis excluded by!**/*.snapsrc/components/select/__tests__/__snapshots__/Select.test.js.snapis excluded by!**/*.snapsrc/components/sidebar-toggle-button/__tests__/__snapshots__/SidebarToggleButton.test.js.snapis excluded by!**/*.snapsrc/components/tooltip/__tests__/__snapshots__/Tooltip.test.tsx.snapis excluded by!**/*.snapsrc/elements/content-sidebar/activity-feed/app-activity/__tests__/__snapshots__/AppActivity.test.js.snapis excluded by!**/*.snapsrc/elements/content-sidebar/activity-feed/comment/__tests__/__snapshots__/Comment.test.js.snapis excluded by!**/*.snapsrc/elements/content-sidebar/activity-feed/task-new/__tests__/__snapshots__/Task.test.js.snapis excluded by!**/*.snapsrc/features/collapsible-sidebar/__tests__/__snapshots__/CollapsibleSidebarMenuItem.test.js.snapis excluded by!**/*.snapsrc/features/left-sidebar/__tests__/__snapshots__/LeftSidebarLinkCallout.test.js.snapis excluded by!**/*.snapsrc/features/pagination/__tests__/__snapshots__/MarkerBasedPagination.test.js.snapis excluded by!**/*.snapsrc/features/pagination/__tests__/__snapshots__/OffsetBasedPagination.test.js.snapis excluded by!**/*.snapsrc/features/sandbox-banner/__tests__/__snapshots__/SandboxBanner.test.tsx.snapis excluded by!**/*.snapsrc/features/unified-share-modal/__tests__/__snapshots__/SharedLinkSection.test.js.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (48)
.flowconfig(1 hunks)package.json(2 hunks)src/components/button-group/ButtonGroup.scss(3 hunks)src/components/checkbox/Checkbox.scss(1 hunks)src/components/context-menu/ContextMenu.tsx(2 hunks)src/components/context-menu/__tests__/ContextMenu.test.tsx(9 hunks)src/components/draft-js-editor/DraftJSEditor.js(1 hunks)src/components/draft-js-editor/DraftJSEditor.scss(1 hunks)src/components/dropdown-menu/DropdownMenu.js(3 hunks)src/components/dropdown-menu/DropdownMenu.scss(1 hunks)src/components/dropdown-menu/__tests__/DropdownMenu.test.js(17 hunks)src/components/flyout/Flyout.js(1 hunks)src/components/flyout/__tests__/Flyout.test.js(20 hunks)src/components/footer-indicator/FooterIndicator.scss(1 hunks)src/components/footer-indicator/FooterIndicator.tsx(1 hunks)src/components/media/Media.scss(1 hunks)src/components/media/MediaMenu.tsx(1 hunks)src/components/menu/MenuItem.scss(1 hunks)src/components/menu/MenuItem.tsx(2 hunks)src/components/pill-selector-dropdown/__tests__/PillSelector.test.js(16 hunks)src/components/radar/RadarAnimation.scss(1 hunks)src/components/radar/RadarAnimation.stories.tsx(1 hunks)src/components/radar/RadarAnimation.tsx(5 hunks)src/components/radar/__tests__/RadarAnimation.test.tsx(2 hunks)src/components/select/Select.js(1 hunks)src/components/text-area/__tests__/TextArea.test.js(2 hunks)src/components/text-input/__tests__/TextInput.test.js(1 hunks)src/components/tooltip/Tooltip.scss(1 hunks)src/components/tooltip/Tooltip.tsx(5 hunks)src/components/tooltip/__tests__/Tooltip.test.tsx(14 hunks)src/elements/content-sidebar/SidebarNavButton.js(3 hunks)src/elements/content-sidebar/SidebarNavButton.scss(4 hunks)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js(1 hunks)src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js(10 hunks)src/elements/content-sidebar/activity-feed/app-activity/AppActivity.js(1 hunks)src/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.js(2 hunks)src/elements/content-sidebar/activity-feed/comment/Comment.js(2 hunks)src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js(6 hunks)src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx(1 hunks)src/elements/content-sidebar/activity-feed/task-new/Task.js(1 hunks)src/elements/content-sidebar/activity-feed/task-new/__tests__/Task.test.js(3 hunks)src/features/collapsible-sidebar/CollapsibleSidebar.scss(1 hunks)src/features/collapsible-sidebar/CollapsibleSidebarMenuItem.js(1 hunks)src/features/left-sidebar/LeftSidebarLinkCallout.js(1 hunks)src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js(3 hunks)src/features/sandbox-banner/SandboxBanner.tsx(1 hunks)src/features/unified-share-modal/SharedLinkSection.js(3 hunks)src/features/unified-share-modal/UnifiedShareModal.scss(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- src/components/menu/MenuItem.scss
- src/features/collapsible-sidebar/CollapsibleSidebar.scss
- src/components/radar/RadarAnimation.stories.tsx
- src/components/menu/MenuItem.tsx
- src/components/draft-js-editor/DraftJSEditor.js
- .flowconfig
- src/components/text-input/tests/TextInput.test.js
- src/components/footer-indicator/FooterIndicator.tsx
- src/components/select/Select.js
- src/features/unified-share-modal/UnifiedShareModal.scss
- src/components/radar/RadarAnimation.scss
- src/components/dropdown-menu/DropdownMenu.scss
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-15T17:04:28.279Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4275
File: src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js:259-260
Timestamp: 2025-09-15T17:04:28.279Z
Learning: When wrapping enzyme mount() tests with IntlProvider for class-based components, use wrapper.find('ComponentName') to access the actual component instance and state, since the wrapper becomes the provider instead of the component.
Applied to files:
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.jssrc/components/context-menu/__tests__/ContextMenu.test.tsxsrc/components/text-area/__tests__/TextArea.test.jssrc/components/radar/__tests__/RadarAnimation.test.tsxsrc/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.jssrc/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.jssrc/components/tooltip/__tests__/Tooltip.test.tsxsrc/components/flyout/__tests__/Flyout.test.js
📚 Learning: 2025-08-19T17:50:47.187Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4228
File: src/elements/content-sidebar/activity-feed/comment/Comment.js:147-148
Timestamp: 2025-08-19T17:50:47.187Z
Learning: In the box-ui-elements codebase, ActivityThreadReplyForm.js and AnnotationThreadCreate.js component files exist but are not actively used/imported anywhere, so they don't need updates for prop propagation.
Applied to files:
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.jssrc/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js
📚 Learning: 2025-09-03T18:10:42.760Z
Learnt from: fpan225
PR: box/box-ui-elements#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/SidebarNavButton.jssrc/elements/content-sidebar/SidebarNavButton.scss
📚 Learning: 2025-09-03T18:10:29.467Z
Learnt from: fpan225
PR: box/box-ui-elements#4239
File: src/elements/content-sidebar/SidebarNav.js:94-106
Timestamp: 2025-09-03T18:10:29.467Z
Learning: In the ContentSidebar CustomSidebarPanel API, the navButtonProps spread should come after explicit data-* attributes to allow users to override analytics tracking attributes like data-resin-target, data-target-id, and data-testid when needed. This is intentional API design for flexibility.
Applied to files:
src/elements/content-sidebar/SidebarNavButton.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sidebar/SidebarNavButton.js
📚 Learning: 2025-08-27T17:03:48.322Z
Learnt from: bxie-box
PR: box/box-ui-elements#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/components/tooltip/__tests__/Tooltip.test.tsx
🧬 Code graph analysis (18)
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (1)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
AnnotationActivity(48-98)
src/elements/content-sidebar/activity-feed/task-new/Task.js (2)
src/elements/common/interactionTargets.js (2)
ACTIVITY_TARGETS(34-59)ACTIVITY_TARGETS(34-59)src/elements/content-sidebar/activity-feed/task-new/messages.js (1)
messages(9-188)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (3)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/common/interactionTargets.js (2)
ACTIVITY_TARGETS(34-59)ACTIVITY_TARGETS(34-59)src/elements/content-sidebar/activity-feed/annotations/messages.js (1)
messages(3-50)
src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (3)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (6)
handleMenuClose(99-99)isConfirmingDelete(63-63)isResolved(84-84)canDelete(79-79)handleDeleteCancel(87-87)handleDeleteConfirm(88-91)src/elements/content-sidebar/activity-feed/comment/Comment.js (4)
canResolve(165-165)isResolved(170-170)canEdit(164-164)canDelete(163-163)src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenuWrapper.js (3)
handleStatusUpdate(69-73)handleEditClick(56-60)handleDeleteClick(51-54)
src/components/tooltip/Tooltip.tsx (2)
src/components/flyout/Flyout.js (2)
tetherProps(454-463)tetherPosition(392-392)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/components/context-menu/ContextMenu.tsx (2)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/components/context-menu/__tests__/ContextMenu.test.tsx (1)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (3)
FakeButton(12-12)FakeMenu(16-20)sandbox(8-8)
src/components/text-area/__tests__/TextArea.test.js (1)
src/components/text-area/TextArea.js (1)
TextArea(33-91)
src/elements/content-sidebar/activity-feed/comment/Comment.js (2)
src/elements/content-sidebar/activity-feed/comment/BaseComment.js (4)
canResolve(143-143)isResolved(147-147)canEdit(142-142)canDelete(141-141)src/elements/content-sidebar/activity-feed/comment/messages.js (1)
messages(9-65)
src/elements/content-sidebar/activity-feed/app-activity/AppActivity.js (1)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
isConfirmingDelete(63-63)
src/components/radar/__tests__/RadarAnimation.test.tsx (3)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (2)
renderWrapper(24-31)findTetherComponent(33-35)src/components/flyout/__tests__/Flyout.test.js (2)
renderWrapper(39-46)findTetherComponent(48-50)src/components/radar/RadarAnimation.tsx (1)
RadarAnimationProps(61-80)
src/components/flyout/Flyout.js (1)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/elements/content-sidebar/SidebarNavButton.js (2)
src/elements/content-sidebar/__tests__/withSidebarAnnotations.rtl.test.js (1)
internalSidebarNavigationHandler(55-55)src/utils/dom.js (1)
isLeftClick(79-81)
src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js (2)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (1)
findTetherComponent(33-35)src/components/flyout/__tests__/Flyout.test.js (1)
findTetherComponent(48-50)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (1)
src/components/flyout/__tests__/Flyout.test.js (4)
renderWrapper(39-46)FakeButton(19-24)findTetherComponent(48-50)sandbox(7-7)
src/features/unified-share-modal/SharedLinkSection.js (1)
src/features/unified-share-modal/messages.js (1)
messages(3-581)
src/components/radar/RadarAnimation.tsx (2)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
tetherProps(123-128)
src/components/flyout/__tests__/Flyout.test.js (1)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (4)
renderWrapper(24-31)FakeButton(12-12)findTetherComponent(33-35)sandbox(8-8)
🪛 Biome (2.1.2)
src/elements/content-sidebar/SidebarNavButton.js
[error] 14-18: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (69)
src/components/footer-indicator/FooterIndicator.scss (1)
39-42: Solid coverage on the tooltip wrapper!I pity the fool who leaves gaps in their tooltip target—this block display keeps the wrapper fillin' the space and ready for action.
src/elements/content-sidebar/SidebarNavButton.scss (1)
62-64: Keep that target block, sucka!Mr. T digs that new
.bcs-NavButton-targetclass — making the tether wrapperdisplay: blockkeeps the nav button hit area solid like before, so no layout foolery sneaks in when the DOM shifts. Good hustle!src/components/pill-selector-dropdown/__tests__/PillSelector.test.js (16)
204-204: I pity the fool who simplified this prop access, but it looks good!The change from a multi-line chained wrapper call to a direct prop access makes the test code cleaner and more readable. Based on learnings, this aligns with react-tether's v2+ API migration from children to render props, where the library changed from using children to renderTarget and renderElement patterns.
280-280: Target selector change looks right, fool!The change from
wrapper.simulate('click')towrapper.find('.bdl-PillSelector').simulate('click')ensures the click event is properly targeted to the PillSelector container element, which is more specific and accurate for testing the component's behavior.
308-312: Event targeting is on point, sucka!The change to target the
.bdl-PillSelectorelement for the keyDown simulation ensures that the backspace handling is tested on the correct element. This is consistent with the other event handling changes throughout the file.
321-325: Consistent event targeting - I don't pity this fool!The keyDown event is now properly targeted to the
.bdl-PillSelectorelement, maintaining consistency with the other test changes. This ensures the backspace behavior is tested correctly.
334-338: Backspace handling test updated correctly!The event simulation is now properly targeted to the
.bdl-PillSelectorcontainer, which is the correct approach for testing pill removal via backspace.
348-352: Immutable pills backspace test looks solid!The keyDown simulation is correctly targeted to the
.bdl-PillSelectorelement for testing backspace behavior with immutable options.
358-362: Edge case handling maintained properly!The test correctly targets the
.bdl-PillSelectorelement when testing the scenario where there are no pills and no input value.
374-378: Arrow navigation testing improved, fool!The ArrowLeft keyDown event is now properly targeted to the
.bdl-PillSelectorelement, ensuring accurate testing of pill navigation behavior.
389-393: First pill selection edge case handled right!The test correctly simulates the ArrowLeft key on the
.bdl-PillSelectorelement when testing the edge case of keeping the first pill selected.
409-413: Last pill selection logic looks good!The ArrowLeft simulation is properly targeted to test the behavior when selecting the last pill with no input value.
422-426: Input value edge case maintained!The test correctly targets the
.bdl-PillSelectorelement when testing that default behavior isn't prevented when the input has a value.
447-451: ArrowRight handling updated consistently!The keyDown simulation for ArrowRight is properly targeted to the
.bdl-PillSelectorelement, maintaining consistency with all the other event handling changes.
466-470: Next pill selection test improved!The ArrowRight event is correctly targeted to test selecting the next pill when the current selection is not the last pill.
477-481: Edge case for no pill selection handled!The test correctly verifies that default behavior isn't prevented when no pill is selected and ArrowRight is pressed.
492-492: Direct prop invocation simplified nicely!The change from a chained wrapper call to direct prop access
wrapper.find('Pill').at(0).prop('onRemove')()makes the test code cleaner and more direct.
502-502: Immutable pill removal test streamlined!The direct prop access pattern is consistently applied for testing the onRemove callback with immutable pills.
src/features/left-sidebar/LeftSidebarLinkCallout.js (1)
81-84: Give that close button a proper label, fool!A screen reader can’t guess what that PlainButton does when it’s nothing but an icon. I pity the fool who ships unlabeled controls. Slap an accessible name on it so everybody knows it’s the close action.
- <PlainButton className="nav-link-callout-close-button" onClick={this.hideCallout}> + <PlainButton + aria-label="Close" + className="nav-link-callout-close-button" + onClick={this.hideCallout} + >src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js (1)
43-56: Prove onClose gets called, fool!
This test still never checks that the close button fires the callback. Swap in a spy and assert it hit once, or this test ain’t worth the electrons. I pity the fool who ships a no-op assertion.const callout = { content: <div>Hi</div>, - onClose: sandbox.mock(), + onClose: sandbox.spy(), }; @@ const btn = tetherComponent.find('.nav-link-callout-close-button').first(); btn.simulate('click'); + sinon.assert.calledOnce(callout.onClose);src/features/collapsible-sidebar/CollapsibleSidebarMenuItem.js (1)
198-205: Nice hook-up of target wrapper class, sucka!Adding
targetWrapperClassNameis consistent with the tether refactor.Confirm
.bdl-CollapsibleSidebar-menuItemToolTipTargetstyles exist in SCSS and don’t regress spacing/hover.src/components/context-menu/ContextMenu.tsx (3)
2-2: VerifyTetherPropstype actually exists in [email protected].Some builds ship no TS types. If it’s missing, this import breaks TS. Fallback to
anyor a local interface if needed.I can craft a minimal
TetherPropsinterface based on the used fields if the library doesn’t export it. Want me to drop that in?
174-186: Render-prop migration looks tight.Target/element wrappers with refs are correct and guarded by
isOpen. Good consistency with Flyout.
169-171: Optional chaining won’t save you on SSR — guarddocumentproperly, fool!Referencing
documentthrows on the server. Use a typeof check.- if (document?.body) { - tetherProps.renderElementTo = document.body; - } + if (typeof document !== 'undefined' && document.body) { + tetherProps.renderElementTo = document.body; + }src/components/flyout/Flyout.js (1)
491-507: Clean migration to renderTarget/renderElement.Inline-block target wrapper and conditional element rendering match react-tether v3 best practices.
src/components/tooltip/Tooltip.tsx (1)
347-401: Fix the render ref types, fool!React-tether 3.0 hands us callback refs, but we typed both helpers as
RefObject. TS rejects that narrower type, so the build faceplants. Let those params acceptReact.Ref<HTMLDivElement>and keep pity off this project.- const renderTooltip = (ref: React.RefObject<HTMLDivElement>) => { + const renderTooltip = (ref: React.Ref<HTMLDivElement>) => { @@ - renderTarget={(ref: React.RefObject<HTMLDivElement>) => { + renderTarget={(ref: React.Ref<HTMLDivElement>) => {src/components/button-group/ButtonGroup.scss (2)
46-47: Selectors updated for wrapped buttons — LGTM, fool!The broader selectors cover targets wrapped by tether/tooltip containers. Good move for consistency.
Also applies to: 68-69, 75-76, 80-81, 89-90
106-106: Opacity normalization — LGTMNumeric form reads cleaner and is consistent.
src/components/media/Media.scss (1)
8-10: Block-level target wrapper — verify no layout regressionsSetting .bdl-DropdownMenu-target to block under .bdl-Media can affect inline flow. Sanity-check menu alignment in Media-body across common breakpoints.
src/components/tooltip/Tooltip.scss (1)
228-230: New target class — LGTM, sucka.inline-block wrapper matches the new renderTarget pattern and keeps layout stable.
src/elements/content-sidebar/SidebarNavButton.js (2)
14-18: Biome false-positive on Flowimport type— check tooling configThis is valid Flow syntax (file is @flow). Make sure Biome ignores Flow-typed JS or is configured accordingly. Don’t let a tool clown you.
85-91: Consistent Tooltip wrapper in both branches — nice workBoth routerDisabled and router paths now use targetWrapperClassName="bcs-NavButton-target". That’s the way, fool. Keeps width/layout consistent.
Based on learnings
Also applies to: 137-142
src/components/tooltip/__tests__/Tooltip.test.tsx (5)
5-5: Keep both mount and shallow; usage is correct.Import change is fine given mixed mount/shallow tests. Don’t overthink it, fool.
71-88: Good probe of tether props via findWhere.Default attachments/constraints assertions match the new renderTarget/renderElement pattern.
180-194: Body element targeting verification is on point.Asserting renderElementTo equals provided bodyEl is the right move.
196-209: Fallback to document.body is validated correctly.Nice negative test for invalid bodyElement.
516-529: Make the keydown-listener test robust by filtering calls, not matching signature.I pity the fool who relies on exact spy args — options vary. Filter for any 'keydown' registration.
- // Keydown listener should not be added since it's commented out - expect(addEventListenerSpy).not.toHaveBeenCalledWith('keydown', expect.anything(), true); + // Keydown listener should not be added at all + const keydownCalls = addEventListenerSpy.mock.calls.filter(call => call[0] === 'keydown'); + expect(keydownCalls.length).toBe(0); addEventListenerSpy.mockRestore();src/components/flyout/__tests__/Flyout.test.js (10)
39-47: Helper renderWrapper/findTetherComponent abstractions are clean.Nice consolidation to mount + probe tether via renderTarget/renderElement. Strong work, fool.
79-91: ARIA expectations are correct for closed state.Accurate wiring of expanded/controls attributes.
93-106: State transition wrapped in act + update — nice.Avoids setState race conditions. Respect.
142-151: Tether defaults asserted properly.Checks for attachment, targetAttachment, classPrefix, enabled=false are solid.
153-165: Enabled toggling verified — good.Visibility drives tether enablement as expected.
168-173: Offset passthrough test is correct.Direct mapping validated.
175-183: Classes mapping verified.Ensures consumer className flows to tether element class. No foolin’.
186-190: Constraints cleared when constrainToScrollParent=false — good.Matches expected behavior.
192-206: Window constraint tests look right.Pinning variant also covered.
259-264: Offset per position assertions are accurate.Good coverage for each position’s computed offset.
package.json (1)
269-269: Dedupe confirmed — [email protected] only version installed
I pity the fool who ships mismatched locks!yarn listshows no other subdeps.src/features/unified-share-modal/SharedLinkSection.js (2)
203-204: LGTM: harmless destructuring tweakNo semantic change. Keep it movin’, fool.
429-431: LGTM: props destructuringReads clean. No issues, sucka.
src/elements/content-sidebar/activity-feed/task-new/Task.js (1)
296-341: Render props migration looks solid (and the missing return is fixed)The Tether renderTarget/renderElement pattern is correct; DeleteConfirmation will render as expected now. Good hustle.
src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (1)
116-129: LGTM: Conditional renderElement is correctExplicit return is there; confirmation renders only when needed. That’s tight.
src/components/dropdown-menu/DropdownMenu.js (4)
16-17: Nice: targetWrapperClassName propGood addition for styling the tether target wrapper.
276-280: LGTM: target wrapper uses class; avoids inline style pitfallsbdl-DropdownMenu-target + optional targetWrapperClassName is the right pattern.
281-287: LGTM: renderElement guarded by isOpenClean and efficient.
274-275: Use a CSS selector string for renderElementTo
renderElementTo is PropTypes.string in react-tether v3.0.3; passing an Element (bodyEl) will break.
Apply:- renderElementTo={bodyEl} + renderElementTo="body"I pity the fool who doesn’t!
src/components/context-menu/__tests__/ContextMenu.test.tsx (4)
56-69: LGTM: mount + instance assertionsUsing mount and reading instance IDs is appropriate here.
84-94: LGTM: act() + update()State flip wrapped in act is correct. No jibber-jabber.
113-119: LGTM: Tether prop assertions via renderTarget/renderElementReasonable way to locate the component post-refactor.
252-254: LGTM: noop when setting isOpen=true againact() around setState is correct. Solid.
src/elements/content-sidebar/activity-feed/comment/Comment.js (4)
66-67: Default prop for onSelect looks good, fool.Matches existing pattern for onDelete/onEdit. No concerns.
189-201: Solid react-tether migration to renderTarget/renderElement.Attachments, constraints, and menu open/close wiring look correct. I pity the fool who breaks positioning — this won’t.
246-259: DeleteConfirmation tether element looks tight.Conditional render, ref wiring, and inline-block wrapper are correct for positioning. Good hustle.
202-221: Fix muddied analytics: distinct resin targets for resolve/unresolve.Both actions use COMMENT_OPTIONS_EDIT, polluting metrics. Add COMMENT_OPTIONS_RESOLVE and COMMENT_OPTIONS_UNRESOLVE and use them here.
Apply this diff here:
- data-resin-target={ACTIVITY_TARGETS.COMMENT_OPTIONS_EDIT} + data-resin-target={ACTIVITY_TARGETS.COMMENT_OPTIONS_UNRESOLVE} ... - data-resin-target={ACTIVITY_TARGETS.COMMENT_OPTIONS_EDIT} + data-resin-target={ACTIVITY_TARGETS.COMMENT_OPTIONS_RESOLVE}And add the new targets in src/elements/common/interactionTargets.js:
// plus existing exports export const ACTIVITY_TARGETS = { // ... COMMENT_OPTIONS_RESOLVE: 'activityfeed-comment-resolve', COMMENT_OPTIONS_UNRESOLVE: 'activityfeed-comment-unresolve', };Run to locate remaining misuses:
#!/bin/bash rg -n -C2 -P "data-resin-target=.*COMMENT_OPTIONS_EDIT" src | sed -n '1,200p'Based on learnings
src/components/radar/__tests__/RadarAnimation.test.tsx (5)
14-16: Finder for tether component is on point.Clean and consistent with other suites. I like it.
78-84: Offset assertion aligned with new API.Inspecting tether prop beats DOM poking. Rock solid.
88-92: Snapshot for tetherElementClassName is fine.Covers class passthrough without overfitting.
98-113: isShown checks via enabled prop are correct.Exactly how v3 exposes visibility. I pity the fool who tests via DOM presence here.
120-123: Instance positioning test is pragmatic.TS ignore is acceptable here to stub the tether ref.
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js
Outdated
Show resolved
Hide resolved
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js
Show resolved
Hide resolved
src/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (2)
124-129: Move that modal class to the renderElement div, fool!The
classNameintetherPropswon't style the rendered element here, sucka. Apply the class directly to the<div ref={ref}>inrenderElementinstead.Based on past review comments.
232-244: Drop the redundant guard inside renderElement, I pity the fool!Line 234 checks
isConfirmingDelete &&again, butrenderElementalready returnsnullwhenisConfirmingDeleteisfalse(line 232). That inner check is redundant, sucka.Apply this diff to clean it up:
renderElement={ref => { return isConfirmingDelete ? ( <div ref={ref}> - {isConfirmingDelete && ( - <DeleteConfirmation - data-resin-component={ACTIVITY_TARGETS.ANNOTATION_OPTIONS} - isOpen={isConfirmingDelete} - message={messages.annotationActivityDeletePrompt} - onDeleteCancel={handleDeleteCancel} - onDeleteConfirm={handleDeleteConfirm} - /> - )} + <DeleteConfirmation + data-resin-component={ACTIVITY_TARGETS.ANNOTATION_OPTIONS} + isOpen={isConfirmingDelete} + message={messages.annotationActivityDeletePrompt} + onDeleteCancel={handleDeleteCancel} + onDeleteConfirm={handleDeleteConfirm} + /> </div> ) : null; }}Based on past review comments.
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (1)
241-245: Add wrapper.update() after that act, sucka!The state flip happens inside
React.actat line 242, but the wrapper ain't refreshed before the assertion at line 245. Drop inwrapper.update()right after the act block so the test proves the menu really toggled, fool!Apply this diff:
React.act(() => { wrapper.find(AnnotationActivityMenu).prop('onDelete')(); }); + wrapper.update(); expect(wrapper.exists(DeleteConfirmation));Based on past review comments.
🧹 Nitpick comments (2)
src/components/checkbox/CheckboxTooltip.tsx (1)
21-31: I pity the fool who wraps every tooltip in its own provider!Sucka, you're creating a new TooltipProvider for each CheckboxTooltip instance. That's inefficient, fool! Blueprint Web's TooltipProvider should typically be placed at a higher level in your component tree (like the app root or a major section container) to manage all tooltips in that subtree, not wrapped around individual tooltips.
Recommended refactor:
Move TooltipProvider to a higher level in your app architecture (e.g., your root component or layout wrapper) and remove it from this component, fool!
Consider the targetWrapperClassName pattern, fool!
The AI summary mentions that the broader migration introduces
targetWrapperClassNameand.bdl-Tooltip-targetclasses across related components. This component doesn't use that pattern, which might be intentional, but it's worth verifying for consistency, sucka!If the pattern should apply here, you could refactor like this:
- <TooltipProvider> - <Tooltip content={tooltip}> - <div className="info-tooltip"> + <Tooltip content={tooltip} targetWrapperClassName="bdl-Tooltip-target"> <IconInfo height={16} title={<FormattedMessage {...messages.checkboxTooltipIconInfoText} />} width={16} /> - </div> </Tooltip> - </TooltipProvider>I pity the fool who doesn't verify consistency! Check if the targetWrapperClassName pattern should be applied here for alignment with the broader refactor, sucka!
src/components/label/InfoIconWithTooltip.tsx (1)
17-23: Confirm Blueprint Tooltip API and refine wrapper usage
contentis the correct prop (TooltipProps definescontent;textremains legacy).- Default positioning differs from
TOP_CENTER; addposition={TooltipPosition.TOP_CENTER}if you need the previous behavior.- Rather than a manual
<span>, passtargetWrapperClassName="info-icon-container"into<Tooltip>to align with the global refactor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/components/footer-indicator/__tests__/__snapshots__/FooterIndicator.test.tsx.snapis excluded by!**/*.snapsrc/components/label/__tests__/__snapshots__/InfoIconWithTooltip.test.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
package.json(2 hunks)src/components/checkbox/CheckboxTooltip.tsx(2 hunks)src/components/footer-indicator/FooterIndicator.tsx(2 hunks)src/components/label/InfoIconWithTooltip.tsx(2 hunks)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js(1 hunks)src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/footer-indicator/FooterIndicator.tsx
- package.json
🧰 Additional context used
🧬 Code graph analysis (2)
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (2)
src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js (1)
getWrapper(21-21)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
AnnotationActivity(49-99)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (3)
src/components/flyout/Flyout.js (1)
tetherProps(454-463)src/elements/common/interactionTargets.js (2)
ACTIVITY_TARGETS(34-59)ACTIVITY_TARGETS(34-59)src/elements/content-sidebar/activity-feed/annotations/messages.js (1)
messages(3-50)
⏰ 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/components/checkbox/CheckboxTooltip.tsx (1)
4-4: I approve this import, fool!Sucka, you migrated to Blueprint Web's Tooltip components correctly. The import statement looks good and aligns with the package structure, fool!
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (2)
112-124: LGTM on these act + update combos, fool!Lines 112-115 and 119-122 correctly wrap state changes in
act()and refresh withwrapper.update()before assertions. Good pattern for mount-based tests, sucka!
256-289: Delete confirmation flow looks solid, I pity the fool who breaks it!Lines 256-263 and 278-285 correctly implement act + update pattern for state-driven interactions. The delete confirmation tests properly verify the DeleteConfirmation component appears and disappears as expected.
src/components/label/InfoIconWithTooltip.tsx (1)
2-2: I pity the fool who forgets to import both components, but you got it right, sucka!The import statement correctly brings in both
TooltipandTooltipProviderfrom@box/blueprint-web, which is necessary for the new composition pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/features/unified-share-modal/UnifiedShareModal.scss (1)
242-248: Potential CSS rule duplication.Lines 242–244 repeat the
.bdl-Tooltip-target { display: block; }styling already defined globally at lines 50–52. If the.shared-link-field-rowcontext is always wrapped with.tooltip-target, the global rule should apply, making the scoped rule redundant. If scoping is intentional (e.g., the field row tooltips do not use.tooltip-targetwrappers), consider adding a comment to clarify the reasoning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/features/unified-share-modal/__tests__/__snapshots__/SharedLinkSection.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/features/unified-share-modal/SharedLinkSection.js(4 hunks)src/features/unified-share-modal/UnifiedShareModal.scss(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/unified-share-modal/SharedLinkSection.js
🧰 Additional context used
🧠 Learnings (1)
📓 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.
⏰ 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). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (2)
src/features/unified-share-modal/UnifiedShareModal.scss (2)
50-56: Verify necessity of tooltip target styling rules.Lines 50–52 introduce a global
.tooltip-target .bdl-Tooltip-target { display: block; }rule. Later, lines 242–244 add a scoped.bdl-Tooltip-target { display: block; }rule within.shared-link-field-row. Consider clarifying whether both rules are necessary or if the global rule (lines 50–52) should suffice for all.bdl-Tooltip-targetelements in the unified share modal context.
213-215: DOM-structure dependency in width workaround.The direct child span selector
> span { width: 100%; }in.shared-link-toggle-rowis a workaround for Tooltip wrapper width issues. This approach is brittle and couples styling to DOM structure. Verify that:
- The component rendering logic for this row consistently wraps content in a
spanelement.- There is no component-level prop (e.g.,
wrapperClassName) that could apply the width styling more robustly.- The width issue cannot be resolved by adjusting the Tooltip wrapper's CSS instead.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (1)
241-245: Add wrapper.update() after React.act() to refresh the Enzyme wrapper.The Enzyme wrapper must be updated after the state change triggered by React.act() so that the assertion on line 245 checks the current DOM state rather than stale state.
Apply this diff:
React.act(() => { wrapper.find(AnnotationActivityMenu).prop('onDelete')(); }); + wrapper.update(); expect(wrapper.exists(DeleteConfirmation));
🧹 Nitpick comments (2)
src/elements/content-sidebar/__tests__/SidebarNav.test.js (2)
64-82: Aria-label assertion for disabled Box AI tab looks correctUsing
aria-disabledplusaria-labeltied todisabledTooltipis aligned with accessibility-focused checks and avoids brittle hover/tooltip DOM assertions. The test can also dropasyncnow that there’s no asynchronous interaction orawaitusage, but that’s purely optional clean-up.- async ({ disabledTooltip, expectedTooltip }) => { + ({ disabledTooltip, expectedTooltip }) => {
84-94: Default Box AI tab aria-label check is appropriate; async can be removedVerifying the default
aria-labelas"Box AI"directly on the button is a solid, implementation-agnostic assertion for the tooltip/accessible name. As above, you can simplify by removingasyncsince the body is fully synchronous.- test('given feature boxai.sidebar.showOnlyNavButton = false, should render box ai tab with default tooltip', async () => { + test('given feature boxai.sidebar.showOnlyNavButton = false, should render box ai tab with default tooltip', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
package.json(2 hunks)src/elements/content-sidebar/SidebarNavButton.js(6 hunks)src/elements/content-sidebar/SidebarNavButton.scss(1 hunks)src/elements/content-sidebar/__tests__/SidebarNav.test.js(1 hunks)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js(1 hunks)src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js
- src/elements/content-sidebar/SidebarNavButton.js
🧰 Additional context used
🧠 Learnings (7)
📓 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-08-19T17:50:47.187Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4228
File: src/elements/content-sidebar/activity-feed/comment/Comment.js:147-148
Timestamp: 2025-08-19T17:50:47.187Z
Learning: In the box-ui-elements codebase, ActivityThreadReplyForm.js and AnnotationThreadCreate.js component files exist but are not actively used/imported anywhere, so they don't need updates for prop propagation.
Applied to files:
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js
📚 Learning: 2025-09-15T17:04:28.279Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js:259-260
Timestamp: 2025-09-15T17:04:28.279Z
Learning: When wrapping enzyme mount() tests with IntlProvider for class-based components, use wrapper.find('ComponentName') to access the actual component instance and state, since the wrapper becomes the provider instead of the component.
Applied to files:
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js
📚 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/SidebarNavButton.scsssrc/elements/content-sidebar/__tests__/SidebarNav.test.js
📚 Learning: 2025-09-03T18:10:29.467Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarNav.js:94-106
Timestamp: 2025-09-03T18:10:29.467Z
Learning: In the ContentSidebar CustomSidebarPanel API, the navButtonProps spread should come after explicit data-* attributes to allow users to override analytics tracking attributes like data-resin-target, data-target-id, and data-testid when needed. This is intentional API design for flexibility.
Applied to files:
src/elements/content-sidebar/SidebarNavButton.scsssrc/elements/content-sidebar/__tests__/SidebarNav.test.js
📚 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/__tests__/SidebarNav.test.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sidebar/__tests__/SidebarNav.test.js
🧬 Code graph analysis (1)
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (2)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
AnnotationActivity(49-99)src/elements/content-sidebar/activity-feed/annotations/messages.js (1)
messages(3-50)
⏰ 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). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (3)
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (3)
2-2: LGTM! Switching to mount for full DOM rendering.The change from shallow to mount rendering is appropriate for testing components that use render props (renderTarget/renderElement) with react-tether 3.0.3, and ensures compatibility with React 18's concurrent features.
Also applies to: 49-49
87-87: LGTM! .first() correctly isolates the target element.When using mount(), multiple AnnotationActivityLink instances may exist in the rendered tree. Calling .first() ensures the test accesses the intended element.
Also applies to: 129-129, 137-137, 436-436, 506-506
112-122: LGTM! Correct usage of React.act() with wrapper.update().All state-changing interactions are properly wrapped in React.act() and followed by wrapper.update(), ensuring that assertions check the current component state after re-rendering.
Also applies to: 256-285, 338-373
| .bcs-NavButton-target { | ||
| display: block; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find SidebarNavButton component files
find src/elements/content-sidebar -name "*SidebarNavButton*" -type fRepository: box/box-ui-elements
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Search for targetWrapperClassName usage in the component
rg "targetWrapperClassName" src/elements/content-sidebar/ -A 3 -B 3Repository: box/box-ui-elements
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check the SidebarNavButton.js implementation
rg "export" src/elements/content-sidebar/SidebarNavButton.js -A 20 | head -60Repository: box/box-ui-elements
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Read the full SidebarNavButton.js file to see the component implementation
cat -n src/elements/content-sidebar/SidebarNavButton.jsRepository: box/box-ui-elements
Length of output: 7404
🏁 Script executed:
#!/bin/bash
# Search for wrapper-related properties or props in SidebarNavButton context
rg "wrapper|target" src/elements/content-sidebar/SidebarNavButton.js -iRepository: box/box-ui-elements
Length of output: 262
🏁 Script executed:
#!/bin/bash
# Search for targetWrapperClassName usage anywhere in the codebase
rg "targetWrapperClassName" src/ --type=jsRepository: box/box-ui-elements
Length of output: 1036
🏁 Script executed:
#!/bin/bash
# Search for other target wrapper classes in SCSS to see the pattern
rg "target\}" src/ --type=scss -B 2Repository: box/box-ui-elements
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for targetWrapperClassName usage pattern with wrapper divs
rg -B 5 -A 5 "targetWrapperClassName" src/components/dropdown-menu/DropdownMenu.jsRepository: box/box-ui-elements
Length of output: 1405
🏁 Script executed:
#!/bin/bash
# Check if there are other instances of target wrapper div patterns
rg "className.*bdl-.*-target" src/ --type=js -B 3 -A 3 | head -50Repository: box/box-ui-elements
Length of output: 765
🏁 Script executed:
#!/bin/bash
# Check SidebarNav.js to understand how SidebarNavButton is used and if targetWrapperClassName is passed
rg -A 20 "SidebarNavButton" src/elements/content-sidebar/SidebarNav.js | head -80Repository: box/box-ui-elements
Length of output: 3443
SidebarNavButton.js is missing the targetWrapperClassName prop implementation.
The CSS class .bcs-NavButton-target is defined but not used by the component. Add targetWrapperClassName?: string to the Props type and wrap the Button element with a div that applies both the base class and the optional prop, following the pattern used in DropdownMenu.js and DraftJSEditor.js. Without this, the SCSS change cannot be utilized by parent components.
🤖 Prompt for AI Agents
In src/elements/content-sidebar/SidebarNavButton.scss around lines 73 to 75, the
CSS class .bcs-NavButton-target is defined but SidebarNavButton.js never applies
it; add a targetWrapperClassName?: string to the component Props type, then wrap
the existing Button element in a div that always has the base class
"bcs-NavButton-target" and conditionally includes the passed
targetWrapperClassName (merge class names); follow the same pattern used in
DropdownMenu.js and DraftJSEditor.js so parent components can supply the extra
class and the SCSS rule takes effect. Ensure TypeScript types and any
defaultProps or prop forwarding are updated accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/features/unified-share-modal/UnifiedShareModal.scss (2)
98-100: Consider using a more maintainable selector.The double class selector
.bdl-Tooltip-target.bdl-Tooltip-targetis a specificity hack. While functional, it could be fragile if the Tooltip component's class structure changes.Consider using a more specific parent selector or adding a unique class name instead:
-.bdl-Tooltip-target.bdl-Tooltip-target { +.bdl-Tooltip-target { display: block; }If additional specificity is truly needed, consider adding a more semantic class to the wrapper.
221-223: Prefer a more specific selector.The
> spanselector is too generic and the comment indicates this is a workaround for a width issue. If the Tooltip component generates a span wrapper, consider targeting it with a more specific class name.-.shared-link-toggle-row { +.shared-link-toggle-row { // ... existing rules ... - > span { - width: 100%; // This fixes a width issue with Tooltip wrapper - } + .bdl-Tooltip-target-wrapper { + width: 100%; + } }Alternatively, if the Tooltip wrapper doesn't provide a specific class, consider adding
targetWrapperClassNameto give it one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/features/unified-share-modal/__tests__/__snapshots__/UnifiedShareForm.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
src/constants.js(1 hunks)src/features/unified-share-modal/UnifiedShareForm.js(1 hunks)src/features/unified-share-modal/UnifiedShareModal.scss(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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-16T14:53:03.538Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/components/form-elements/draft-js-mention-selector/index.js:4-4
Timestamp: 2025-09-16T14:53:03.538Z
Learning: The createMentionSelectorState export in src/components/form-elements/draft-js-mention-selector/index.js was confirmed to be internal-only within the box-ui-elements package, with no external references or documentation, making the breaking rename to createMentionTimestampSelectorState safe.
Applied to files:
src/features/unified-share-modal/UnifiedShareForm.js
⏰ 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). (3)
- GitHub Check: lint_test_build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (6)
src/features/unified-share-modal/UnifiedShareModal.scss (5)
50-52: LGTM!This CSS rule correctly supports the
targetWrapperClassNameprop pattern by ensuring tooltip targets with thewrapper-blockclass render as block-level elements.
54-56: LGTM!This flex layout rule appropriately controls the sizing of the FTUX toggle tooltip target.
195-197: LGTM!The flex properties appropriately control the sizing of the tooltip target within the toggle row's flex layout.
250-252: LGTM!This rule ensures tooltip targets in the shared link field row display correctly as block-level elements.
254-256: LGTM!This flex rule appropriately sizes the tooltip wrapper using a specific class name, which is a good practice.
src/features/unified-share-modal/UnifiedShareForm.js (1)
491-491: react-tether version 3.0.3 is confirmed and already in use.The addition of
targetWrapperClassName: 'wrapper-block'is correct and aligns with the Tooltip wrapper refactoring pattern. [email protected] is specified in package.json and locked in yarn.lock with integrity verification.
…ther-upgrade-3.0.3
jfox-box
force-pushed
the
react-tether-upgrade-3.0.3
branch
from
d12cb44 to
61be675
Compare
| > .btn, | ||
| > * > .btn { |
There was a problem hiding this comment.
We style both the button and the child because we conditionally render the button inside a tooltip (when disabled)
| > .info-tooltip, | ||
| > * > .info-tooltip { |
There was a problem hiding this comment.
Same here, we render a tooltip wrapper if disabled
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/context-menu/__tests__/ContextMenu.test.tsx (1)
296-299: Address the previously flagged brittle test expectation.This test uses
withArgs('contextmenu').never()andwithArgs('click').never(), which won't catch calls with additional arguments (like the capture flag). A past review comment correctly identified this as brittle and suggested using spies with call filtering instead.🔎 Recommended fix using spies
- const documentMock = sandbox.mock(document); - documentMock.expects('removeEventListener').withArgs('contextmenu').never(); - documentMock.expects('removeEventListener').withArgs('click').never(); - wrapper.unmount(); + + const removeSpy = sandbox.spy(document, 'removeEventListener'); + const contextmenuCalls = removeSpy.getCalls().filter(call => call.args[0] === 'contextmenu'); + const clickCalls = removeSpy.getCalls().filter(call => call.args[0] === 'click'); + expect(contextmenuCalls.length).toBe(0); + expect(clickCalls.length).toBe(0);
🧹 Nitpick comments (6)
src/elements/content-sidebar/activity-feed/app-activity/AppActivity.scss (1)
7-7: Good formatting improvement.Adding the leading zero (
0.4instead of.4) follows CSS best practices and improves code consistency.src/components/text-area/__tests__/TextArea.test.js (1)
74-80: Test migration looks good; consider adding cleanup.The migration from
shallowtomountand the simplifiedTetherComponentquery approach correctly align with the react-tether 3.0.3 upgrade. The test logic properly verifies thattooltipTetherClassNameis passed through to the TetherComponent'sclassNameprop.Consider adding cleanup after the mounted component to prevent potential memory leaks in the test suite:
🔎 Optional cleanup addition
test('should render Tooltip with tetherElementClassName', () => { const className = 'tether-element-class-name'; const wrapper = mount(<TextArea error="error" label="label" tooltipTetherClassName={className} />); const tetherEl = wrapper.find(TetherComponent); expect(tetherEl.prop('className')).toBe(className); + wrapper.unmount(); });src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
208-228: Consider using a CSS class instead of inline style for consistency.Other components in this PR (Task.js, Comment.js, BaseCommentMenu.tsx) use dedicated CSS classes like
bcs-Task-menuTargetandbcs-Comment-menuTargetfor therenderTargetwrapper. Using inline styles here creates inconsistency.🔎 Suggested change
Add to
AnnotationActivity.scss:.bcs-AnnotationActivity-menuTarget { display: inline-block; }Then update the component:
renderTarget={ref => ( - <div ref={ref} style={{ display: 'inline-block' }}> + <div ref={ref} className="bcs-AnnotationActivity-menuTarget"> {isMenuVisible && (src/elements/content-sidebar/activity-feed/task-new/Task.js (1)
328-341: Consider applying modal class to renderElement wrapper for z-index consistency.Similar to the observation in BaseCommentMenu.tsx, the
bcs-Task-deleteConfirmationModalclass is on theTetherComponentbut not on therenderElementwrapper div. For consistent z-index behavior with AnnotationActivity.js, consider adding the class here as well.🔎 Suggested change
renderElement={ref => { return isConfirmingDelete ? ( - <div ref={ref}> + <div ref={ref} className="bcs-Task-deleteConfirmationModal"> <DeleteConfirmationsrc/elements/content-sidebar/activity-feed/comment/Comment.js (1)
246-259: Consider applying modal class to renderElement wrapper for z-index consistency.For consistency with AnnotationActivity.js (which applies the class on line 232), the modal class should be applied to the
renderElementwrapper div to ensure z-index styling works correctly.🔎 Suggested change
renderElement={ref => isConfirmingDelete ? ( - <div ref={ref}> + <div ref={ref} className="bcs-Comment-deleteConfirmationModal"> <DeleteConfirmationsrc/components/flyout/__tests__/Flyout.test.js (1)
139-262: TetherComponent prop assertions look correct.The tests properly verify that
TetherComponentreceives the correct props for different configurations (attachment, targetAttachment, constraints, offset, classes). The migration from checking Flyout wrapper props to checking TetherComponent props aligns with the react-tether 3.0.3 upgrade.Optional: Make test description more specific in forEach loop
At line 256, the test description could include the position being tested to make test output more informative:
- ].forEach(({ position, offset }) => { - test('should set tether offset correctly when offset props is not passed in', () => { + ].forEach(({ position, offset }) => { + test(`should set tether offset correctly for position ${position} when offset prop is not passed in`, () => { const wrapper = renderWrapper({ position });This would make failing tests easier to identify by position.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
package.jsonsrc/components/context-menu/__tests__/ContextMenu.test.tsxsrc/components/dropdown-menu/__tests__/DropdownMenu.test.jssrc/components/flyout/Flyout.jssrc/components/flyout/__tests__/Flyout.test.jssrc/components/radar/__tests__/RadarAnimation.test.tsxsrc/components/text-area/__tests__/TextArea.test.jssrc/components/text-input/__tests__/TextInput.test.jssrc/components/tooltip/__tests__/Tooltip.test.tsxsrc/elements/content-sidebar/__tests__/SidebarNav.test.jssrc/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.jssrc/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.jssrc/elements/content-sidebar/activity-feed/app-activity/AppActivity.jssrc/elements/content-sidebar/activity-feed/app-activity/AppActivity.scsssrc/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.jssrc/elements/content-sidebar/activity-feed/comment/Comment.jssrc/elements/content-sidebar/activity-feed/comment/Comment.scsssrc/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.jssrc/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.scsssrc/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsxsrc/elements/content-sidebar/activity-feed/task-new/Task.jssrc/elements/content-sidebar/activity-feed/task-new/Task.scsssrc/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.jssrc/features/shared-link-modal/EmailSharedLink.scsssrc/features/shared-link-modal/SharedLink.scsssrc/features/shared-link-settings-modal/SharedLinkSettingsModal.scss
🚧 Files skipped from review as they are similar to previous changes (8)
- src/elements/content-sidebar/tests/SidebarNav.test.js
- src/elements/content-sidebar/activity-feed/annotations/tests/AnnotationActivity.test.js
- src/elements/content-sidebar/activity-feed/app-activity/AppActivity.js
- src/components/text-input/tests/TextInput.test.js
- src/components/flyout/Flyout.js
- src/elements/content-sidebar/activity-feed/comment/tests/Comment.test.js
- src/features/left-sidebar/tests/LeftSidebarLinkCallout.test.js
- src/components/radar/tests/RadarAnimation.test.tsx
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-08-19T17:50:47.187Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4228
File: src/elements/content-sidebar/activity-feed/comment/Comment.js:147-148
Timestamp: 2025-08-19T17:50:47.187Z
Learning: In the box-ui-elements codebase, ActivityThreadReplyForm.js and AnnotationThreadCreate.js component files exist but are not actively used/imported anywhere, so they don't need updates for prop propagation.
Applied to files:
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js
📚 Learning: 2025-09-15T17:04:28.279Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/elements/content-sidebar/activity-feed/comment/__tests__/Comment.test.js:259-260
Timestamp: 2025-09-15T17:04:28.279Z
Learning: When wrapping enzyme mount() tests with IntlProvider for class-based components, use wrapper.find('ComponentName') to access the actual component instance and state, since the wrapper becomes the provider instead of the component.
Applied to files:
src/components/text-area/__tests__/TextArea.test.jssrc/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.jssrc/components/tooltip/__tests__/Tooltip.test.tsxsrc/components/dropdown-menu/__tests__/DropdownMenu.test.jssrc/components/context-menu/__tests__/ContextMenu.test.tsxsrc/components/flyout/__tests__/Flyout.test.js
📚 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/components/text-area/__tests__/TextArea.test.jssrc/components/tooltip/__tests__/Tooltip.test.tsxsrc/components/dropdown-menu/__tests__/DropdownMenu.test.js
📚 Learning: 2025-09-16T14:53:03.538Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/components/form-elements/draft-js-mention-selector/index.js:4-4
Timestamp: 2025-09-16T14:53:03.538Z
Learning: The createMentionSelectorState export in src/components/form-elements/draft-js-mention-selector/index.js was confirmed to be internal-only within the box-ui-elements package, with no external references or documentation, making the breaking rename to createMentionTimestampSelectorState safe.
Applied to files:
src/elements/content-sidebar/activity-feed/comment/Comment.jssrc/features/shared-link-modal/EmailSharedLink.scss
📚 Learning: 2025-09-16T14:53:03.538Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/components/form-elements/draft-js-mention-selector/index.js:4-4
Timestamp: 2025-09-16T14:53:03.538Z
Learning: The createMentionSelectorState export in src/components/form-elements/draft-js-mention-selector/index.js was an internal-only API, not used by external consumers, so breaking changes during rename to createMentionTimestampSelectorState are acceptable.
Applied to files:
src/elements/content-sidebar/activity-feed/comment/Comment.js
📚 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/activity-feed/comment/Comment.js
📚 Learning: 2025-08-19T17:44:59.234Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4228
File: src/elements/content-sidebar/activity-feed/activity-feed/ActiveState.js:185-186
Timestamp: 2025-08-19T17:44:59.234Z
Learning: In ActiveState.js, ActivityThread is only rendered when hasNewThreadedReplies is false. When hasNewThreadedReplies is true (newer threaded replies approach), BaseComment is rendered directly instead of ActivityThread. This means features using the newer threaded replies approach bypass ActivityThread entirely.
Applied to files:
src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx
📚 Learning: 2025-08-15T14:42:01.840Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 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-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 4224
File: package.json:296-297
Timestamp: 2025-08-12T18:04:17.698Z
Learning: In the box-ui-elements project, the team is comfortable with raising peerDependency minimum versions when upgrading blueprint-web packages, even if it's a breaking change for consumers.
Applied to files:
package.json
🧬 Code graph analysis (9)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (3)
src/elements/content-sidebar/activity-feed/comment/Comment.js (4)
isMenuVisible(169-169)canDelete(163-163)canEdit(164-164)canResolve(165-165)src/elements/content-sidebar/activity-feed/task-new/Task.js (1)
isMenuVisible(268-268)src/elements/content-sidebar/activity-feed/comment/BaseComment.js (4)
isMenuVisible(146-146)canDelete(141-141)canEdit(142-142)canResolve(143-143)
src/elements/content-sidebar/activity-feed/task-new/Task.js (4)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
isConfirmingDelete(64-64)src/elements/common/interactionTargets.js (2)
ACTIVITY_TARGETS(34-59)ACTIVITY_TARGETS(34-59)src/api/Feed.js (1)
permissions(196-196)src/elements/content-sidebar/activity-feed/task-new/messages.js (1)
messages(9-188)
src/components/text-area/__tests__/TextArea.test.js (1)
src/components/text-area/TextArea.js (1)
TextArea(33-91)
src/elements/content-sidebar/activity-feed/comment/Comment.js (3)
src/components/dropdown-menu/DropdownMenu.js (1)
noop(97-97)src/elements/content-sidebar/activity-feed/comment/BaseComment.js (4)
canResolve(143-143)isResolved(147-147)canEdit(142-142)canDelete(141-141)src/elements/content-sidebar/activity-feed/comment/messages.js (1)
messages(9-65)
src/components/tooltip/__tests__/Tooltip.test.tsx (1)
src/components/tooltip/index.ts (1)
TooltipTheme(5-5)
src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (2)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (5)
isConfirmingDelete(64-64)isResolved(85-85)canDelete(80-80)handleDeleteCancel(88-88)handleDeleteConfirm(89-92)src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenuWrapper.js (3)
handleStatusUpdate(69-73)handleEditClick(56-60)handleDeleteClick(51-54)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (3)
src/components/flyout/__tests__/Flyout.test.js (3)
renderWrapper(40-47)FakeButton(20-25)sandbox(8-8)src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js (1)
sandbox(9-9)src/components/pill-selector-dropdown/__tests__/PillSelector.test.js (1)
sandbox(8-8)
src/components/context-menu/__tests__/ContextMenu.test.tsx (4)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (3)
FakeButton(13-13)FakeMenu(17-21)sandbox(9-9)src/components/flyout/__tests__/Flyout.test.js (2)
FakeButton(20-25)sandbox(8-8)src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js (1)
sandbox(9-9)src/components/pill-selector-dropdown/__tests__/PillSelector.test.js (1)
sandbox(8-8)
src/components/flyout/__tests__/Flyout.test.js (3)
src/components/dropdown-menu/__tests__/DropdownMenu.test.js (3)
renderWrapper(25-32)FakeButton(13-13)sandbox(9-9)src/features/left-sidebar/__tests__/LeftSidebarLinkCallout.test.js (1)
sandbox(9-9)src/components/pill-selector-dropdown/__tests__/PillSelector.test.js (1)
sandbox(8-8)
⏰ 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). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (29)
src/features/shared-link-modal/EmailSharedLink.scss (2)
5-7: LGTM! Tooltip target wrapper styling is well-scoped.The
.bdl-Tooltip-target { display: block; }rules are appropriately scoped within their parent containers (.bdl-PillSelectorDropdownand.text-area-container). This supports the newtargetWrapperClassNamepattern introduced with the react-tether 3.0.3 upgrade and ensures proper layout for tooltip targets.Also applies to: 16-18
22-22: Nice improvement: normalized transition duration.The change from
.2sto0.2sfollows CSS best practices by including a leading zero, improving code consistency.src/features/shared-link-modal/SharedLink.scss (1)
31-35: LGTM! Consistent tooltip target wrapper styling.The
.shared-link-container .bdl-Tooltip-target { display: block; }rule is well-scoped and aligns with the broader pattern across the codebase for handling tooltip target wrappers introduced with the react-tether 3.0.3 upgrade.src/elements/content-sidebar/activity-feed/app-activity/AppActivity.scss (1)
15-17: CSS class correctly integrated with react-tether upgrade.The new
.bcs-AppActivity-confirmTargetclass is properly referenced in AppActivity.js (line 150) within the Tether component'srenderTargetprop. Thedisplay: inline-blockstyling is appropriate for positioning the menu anchor element. Implementation is correct.src/components/text-area/__tests__/TextArea.test.js (1)
1-3: LGTM! Imports updated for mount testing.The addition of
mountfrom enzyme andTetherComponentfrom react-tether correctly supports the test migration to full DOM mounting, which is necessary to access and verify the TetherComponent in the updated react-tether API.src/elements/content-sidebar/activity-feed/task-new/Task.scss (1)
57-60: LGTM!The new
.bcs-Task-menuTargetclass correctly supports therenderTargetwrapper pattern introduced in Task.js. Theinline-blockdisplay ensures proper positioning for the tethered menu.src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.scss (1)
5-8: LGTM!The new
.bcs-BaseCommentMenu-targetclass aligns with therenderTargetwrapper in BaseCommentMenu.tsx, maintaining consistency with the tether target styling pattern used across the codebase.src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (1)
230-243: LGTM!The
renderElementimplementation correctly applies the modal class to the wrapper div and conditionally renders theDeleteConfirmationonly whenisConfirmingDeleteis true. This addresses the concerns from the past review.src/elements/content-sidebar/activity-feed/comment/components/BaseCommentMenu.tsx (2)
52-56: Verify z-index styling is applied correctly.The
className="bcs-Comment-deleteConfirmationModal"onTetherComponentapplies to the tether wrapper element. However, for consistency with other components (e.g., AnnotationActivity.js line 232), the modal class should also be applied to therenderElementwrapper div to ensure z-index styling works as expected.🔎 Suggested change
renderElement={ref => { return isConfirmingDelete ? ( - <div ref={ref}> + <div ref={ref} className="bcs-Comment-deleteConfirmationModal"> <DeleteConfirmation
57-114: LGTM!The
renderTargetimplementation correctly wrapsMedia.Menuwith the target class and preserves all menu items with their respective handlers, permissions checks, and analytics targets.src/elements/content-sidebar/activity-feed/task-new/Task.js (1)
296-327: LGTM!The
renderTargetimplementation correctly wrapsMedia.Menuwith thebcs-Task-menuTargetclass and preserves menu items with proper permission checks and analytics targets.src/elements/content-sidebar/activity-feed/comment/Comment.js (2)
62-67: LGTM!The new
onSelectdefault prop withnoopfallback is correctly added to support selection state signaling for the menu and delete confirmation interactions.
189-244: LGTM!The
renderTargetimplementation correctly wrapsMedia.Menuwith the target class and integratesdropdownPropsfor menu open/close handlers. Menu items preserve proper permission checks and analytics targets.src/elements/content-sidebar/activity-feed/comment/Comment.scss (2)
16-23: LGTM!The numeric value normalization (
.33s→0.33s,.4→0.4) improves consistency with standard CSS conventions.
38-41: LGTM!The new
.bcs-Comment-menuTargetclass correctly supports therenderTargetwrapper pattern in Comment.js and maintains consistency with the styling approach used across other components in this PR.src/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.js (1)
78-94: Selector updates and mount usage are appropriate for tethered menu.The switch to
mount()and the updated selector (button.bdl-Media-menu) correctly align with the new react-tether 3.x render-props API. The explicitpermissionsprops appropriately test both user-created activities (line 81) and permission-based access (line 90).These changes will work correctly once the IntlProvider wrapper is added to
renderWithMount.src/components/flyout/__tests__/Flyout.test.js (3)
1-47: LGTM! Test setup correctly migrated to react-tether 3.0.3.The import of
TetherComponentand the newrenderWrapperhelper properly support testing with the upgraded react-tether version. Usingmount()instead ofshallow()is the right choice sinceTetherComponentrequires full DOM rendering.
265-527: Event handler tests correctly updated for the new testing approach.The tests properly use
act()for state updates and leveragerenderWrapperfor consistent mounting. The test coverage for event handlers (click, hover, keyboard) is comprehensive and correctly verifies behavior through component state and method mocking.
529-909: Lifecycle and DOM integration tests properly migrated.The tests requiring body mounting maintain proper setup/cleanup, and the component lifecycle tests (
componentDidUpdate,componentWillUnmount) correctly verify event listener registration/deregistration. TheisResponsivetest appropriately checks theTetherComponentclasses prop, consistent with the upgraded API.src/components/dropdown-menu/__tests__/DropdownMenu.test.js (3)
1-32: LGTM! Test setup correctly migrated to mount rendering.The test suite has been properly updated to use full DOM mounting with
renderWrapperand importsTetherComponentfor inspection. This aligns with the react-tether v3 upgrade and enables proper testing of the tether integration.
89-91: LGTM! Proper use of act() for state updates.State changes via
openMenuAndSetFocusIndexare correctly wrapped inact()followed bywrapper.update()to ensure React state and DOM are synchronized before assertions. This pattern is consistently applied throughout the test suite.Also applies to: 120-122, 210-212
137-145: LGTM! TetherComponent props correctly verified.Tests now inspect
TetherComponentprops directly (e.g.,renderElementTo,attachment,targetAttachment,constraints) instead of wrapper props. This correctly validates the react-tether v3 API usage, including the migration frombodyElementtorenderElementTo.Also applies to: 150-158, 173-181
src/components/tooltip/__tests__/Tooltip.test.tsx (4)
73-96: LGTM! Default component test correctly migrated to mount.The test now uses
mountand properly inspectsTetherComponentprops (attachment,constraints,targetAttachment) alongside the button element props. This validates the react-tether v3 integration while maintaining coverage of event handler wiring.
110-124: LGTM! Interactive tooltip test correctly uses mount rendering.The test properly simulates
mouseenteron the mounted button, then queries for the tooltip usingdata-testidand verifies its properties (CSS class, aria-describedby, content). This validates the tooltip show/hide behavior with real DOM interactions.
184-202: LGTM! renderElementTo prop correctly verified.Tests validate that
TetherComponentreceives the correctrenderElementToprop for both valid HTMLElement values and invalid inputs (falling back todocument.body). This ensures proper react-tether v3 API usage.
447-458: LGTM! Event handler tests correctly use mount rendering.Tests for
handleMouseEnter,handleMouseLeave,handleFocus, andhandleBlurproperly usemountto simulate events on the button element and verify both state changes and original handler invocations via sinon spies. This ensures event wiring works correctly with the new tether integration.Also applies to: 462-474, 478-489, 493-505
src/components/context-menu/__tests__/ContextMenu.test.tsx (2)
57-70: LGTM! Test setup correctly uses mount rendering with act().Tests properly use
mountwith TypeScript annotations and wrapsetStatecalls inact()followed bywrapper.update(). This ensures React lifecycle and DOM state are synchronized before assertions, aligning with the react-tether v3 migration.Also applies to: 85-104
114-119: LGTM! TetherComponent props correctly verified.Test inspects
TetherComponentprops directly (attachment,targetAttachment,constraints) to validate the react-tether v3 API integration. This correctly validates the tether configuration.package.json (1)
269-269: react-tether ^3.0.3 is valid and secure—no action needed.Version 3.0.3 exists as the latest release (published October 2023), has no security vulnerabilities per npm audit and Snyk, and is consistently specified across dependencies and peerDependencies.
| const renderWithMount = (props = {}) => | ||
| mount(<AppActivity isPending={false} onDelete={jest.fn()} {...fakeAppActivity} {...props} />); |
There was a problem hiding this comment.
Critical: IntlProvider wrapper still missing despite past review.
The renderWithMount helper mounts the injectIntl-wrapped AppActivity directly, which will throw "Could not find required intl object" errors at runtime. This issue was flagged in a previous review and marked as addressed in commit d4e2482, but the fix is not present in the current code.
🔎 Proposed fix
+import { IntlProvider } from 'react-intl';
+
const renderWithMount = (props = {}) =>
- mount(<AppActivity isPending={false} onDelete={jest.fn()} {...fakeAppActivity} {...props} />);
+ mount(
+ <IntlProvider locale="en">
+ <AppActivity isPending={false} onDelete={jest.fn()} {...fakeAppActivity} {...props} />
+ </IntlProvider>
+ );Note: Based on learnings, when wrapping with IntlProvider you may need to use wrapper.find('AppActivity') to access the actual component instance if you need to inspect component state or props.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const renderWithMount = (props = {}) => | |
| mount(<AppActivity isPending={false} onDelete={jest.fn()} {...fakeAppActivity} {...props} />); | |
| import { IntlProvider } from 'react-intl'; | |
| const renderWithMount = (props = {}) => | |
| mount( | |
| <IntlProvider locale="en"> | |
| <AppActivity isPending={false} onDelete={jest.fn()} {...fakeAppActivity} {...props} /> | |
| </IntlProvider> | |
| ); |
🤖 Prompt for AI Agents
In
src/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.js
around lines 30-31, the test helper mounts the injectIntl-wrapped AppActivity
directly causing "Could not find required `intl` object" runtime errors; wrap
the mounted component in an <IntlProvider locale="en"> (or the app's default
locale) when calling mount so the injected intl context is provided, update
renderWithMount to mount <IntlProvider><AppActivity ... /></IntlProvider>, and
if the test needs to inspect the wrapped component instance or props use
wrapper.find('AppActivity') to access it.
|
|
||
| .vanity-name-content { | ||
| .bdl-Tooltip-target { | ||
| display: block; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the SharedLinkSettingsModal component renders the tooltip target wrapper
# and check for any layout-related concerns with display: block
# Find the component file(s) for SharedLinkSettingsModal
echo "=== Finding SharedLinkSettingsModal component files ==="
fd -e js -e jsx -e ts -e tsx SharedLinkSettingsModal
# Search for usage of bdl-Tooltip-target class and vanity-name-content within the component
echo -e "\n=== Searching for bdl-Tooltip-target and vanity-name-content usage ==="
rg -n -C5 "bdl-Tooltip-target|vanity-name-content" --type-list | head -20
rg -n -C5 "bdl-Tooltip-target|vanity-name-content" src/features/shared-link-settings-modal/ 2>/dev/null | head -100
# Check if TooltipProvider or Tooltip with renderTarget is used in the component
echo -e "\n=== Checking for Tooltip/TooltipProvider with renderTarget ==="
rg -n -C3 "renderTarget|TooltipProvider" src/features/shared-link-settings-modal/ 2>/dev/null | head -100
# Also check the SCSS file itself to understand the full context
echo -e "\n=== Checking SharedLinkSettingsModal.scss full context ==="
cat -n src/features/shared-link-settings-modal/SharedLinkSettingsModal.scss | sed -n '40,65p'Repository: box/box-ui-elements
Length of output: 3967
🏁 Script executed:
#!/bin/bash
# Check the VanityNameSection component for Tooltip usage
echo "=== VanityNameSection.js full content ==="
cat -n src/features/shared-link-settings-modal/VanityNameSection.js
echo -e "\n\n=== Searching for Tooltip in component files ==="
rg -n "Tooltip" src/features/shared-link-settings-modal/*.js 2>/dev/null
echo -e "\n\n=== Searching for bdl-Tooltip across entire component directory ==="
rg -r "bdl-Tooltip" src/features/shared-link-settings-modal/ 2>/dev/nullRepository: box/box-ui-elements
Length of output: 3812
🏁 Script executed:
#!/bin/bash
# Check AllowDownloadSection.js to see how Tooltip is correctly used
echo "=== AllowDownloadSection.js Tooltip usage (lines 60-90) ==="
sed -n '60,90p' src/features/shared-link-settings-modal/AllowDownloadSection.js
echo -e "\n\n=== Check if vanity-name-content should have a tooltip ==="
rg -n -B5 -A5 "vanity-name" src/features/shared-link-settings-modal/*.js | head -80
echo -e "\n\n=== Check git changes for this file ==="
git diff HEAD -- src/features/shared-link-settings-modal/SharedLinkSettingsModal.scss 2>/dev/null | head -50
git diff HEAD -- src/features/shared-link-settings-modal/VanityNameSection.js 2>/dev/null | head -50
echo -e "\n\n=== Check broader context of what changed in this PR ==="
git log --oneline -n 20 2>/dev/null | grep -i "tether\|react-tether" | head -5
</parameter>Repository: box/box-ui-elements
Length of output: 3921
🏁 Script executed:
#!/bin/bash
# Search all JS/TSX files for vanity-name-content usage
echo "=== All references to vanity-name-content in JS files ==="
rg -n "vanity-name-content" src/ -t js -t jsx -t ts -t tsx
echo -e "\n\n=== Check if Tooltip wraps vanity-name-section elsewhere ==="
rg -n -B3 -A3 "vanity-name-section" src/features/shared-link-settings-modal/SharedLinkSettingsModal.js
echo -e "\n\n=== Confirm Tooltip imports in shared-link-settings-modal ==="
rg -n "import.*Tooltip" src/features/shared-link-settings-modal/Repository: box/box-ui-elements
Length of output: 406
🏁 Script executed:
#!/bin/bash
# Check SharedLinkSettingsModal.js for VanityNameSection usage
echo "=== SharedLinkSettingsModal.js full content (first 150 lines) ==="
head -150 src/features/shared-link-settings-modal/SharedLinkSettingsModal.js
echo -e "\n\n=== Check for any Tooltip usage in SharedLinkSettingsModal.js ==="
rg -n "Tooltip" src/features/shared-link-settings-modal/SharedLinkSettingsModal.jsRepository: box/box-ui-elements
Length of output: 6485
🏁 Script executed:
#!/bin/bash
# Search for where VanityNameSection is rendered in SharedLinkSettingsModal.js
echo "=== Where VanityNameSection is used in render/return ==="
rg -n "VanityNameSection" src/features/shared-link-settings-modal/SharedLinkSettingsModal.js -A 5 -B 2
# Also check the entire file length to understand scope
echo -e "\n\n=== Total lines in SharedLinkSettingsModal.js ==="
wc -l src/features/shared-link-settings-modal/SharedLinkSettingsModal.js
# Get the render method
echo -e "\n\n=== SharedLinkSettingsModal.js render/JSX section ==="
tail -200 src/features/shared-link-settings-modal/SharedLinkSettingsModal.js | head -100Repository: box/box-ui-elements
Length of output: 6016
Remove dead CSS rule or implement the Tooltip wrapper in VanityNameSection.
The CSS selector .vanity-name-content .bdl-Tooltip-target { display: block; } targets an element that is never rendered. VanityNameSection does not use a Tooltip component, so .bdl-Tooltip-target is never present as a child of .vanity-name-content. Either remove this CSS rule or update VanityNameSection to actually wrap content in a Tooltip component if that was the intended change.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.