feat(react-tether): Upgrade react-tether to 3.0.3 #4278
Conversation
WalkthroughI pity the fool: upgrades Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Component (Tooltip/Flyout/Menu/Radar)
participant Tether as TetherComponent
participant Body as document.body
User->>UI: trigger open/hover/interaction
UI->>Tether: register renderTarget(cb) & renderElement(cb)
Tether->>UI: invoke renderTarget to obtain target node
UI-->>Tether: return <div class="bdl-*-target" ref=targetRef>target</div>
alt overlay visible
Tether->>UI: invoke renderElement to obtain overlay node
UI-->>Tether: return <div ref=elementRef>overlay</div>
Tether->>Body: mount overlay to renderElementTo (document.body)
Note over Tether,Body: Tether positions overlay relative to target
else overlay hidden
UI-->>Tether: renderElement returns null or enabled=false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 react-tether@3.0.3 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 react-tether@3.x 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 react-tether@3.0.3 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.
| <PlainButton className="nav-link-callout-close-button" onClick={this.hideCallout}> | ||
| <IconClose color="#fff" height={16} width={16} /> | ||
| </PlainButton> |
There was a problem hiding this comment.
Add an accessible label to the close button, sucka.
Screen readers won’t announce intent with just an icon. I pity the fool who ships unlabeled controls.
Apply:
- <PlainButton className="nav-link-callout-close-button" onClick={this.hideCallout}>
+ <PlainButton
+ className="nav-link-callout-close-button"
+ onClick={this.hideCallout}
+ aria-label="Close"
+ >📝 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.
| <PlainButton className="nav-link-callout-close-button" onClick={this.hideCallout}> | |
| <IconClose color="#fff" height={16} width={16} /> | |
| </PlainButton> | |
| <PlainButton | |
| className="nav-link-callout-close-button" | |
| onClick={this.hideCallout} | |
| aria-label="Close" | |
| > | |
| <IconClose color="#fff" height={16} width={16} /> | |
| </PlainButton> |
🤖 Prompt for AI Agents
In src/features/left-sidebar/LeftSidebarLinkCallout.js around lines 81 to 83,
the close PlainButton only contains an icon so screen readers cannot announce
its purpose; add an accessible name by supplying an aria-label (e.g.
aria-label="Close" or aria-label={t('Close')} if using i18n) or an
aria-labelledby pointing to an offscreen text node, ensuring the button has a
clear accessible name for screen readers.
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 react-tether@3.0.3.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 — react-tether@3.0.3 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.
| const documentMock = sandbox.mock(document); | ||
| documentMock | ||
| .expects('removeEventListener') | ||
| .withArgs('contextmenu') | ||
| .never(); | ||
| documentMock | ||
| .expects('removeEventListener') | ||
| .withArgs('click') | ||
| .never(); | ||
| documentMock.expects('removeEventListener').withArgs('contextmenu').never(); | ||
| documentMock.expects('removeEventListener').withArgs('click').never(); | ||
|
|
There was a problem hiding this comment.
Fix brittle sinon expectations — they can miss real calls
withArgs('contextmenu').never() won’t catch calls with extra args (capture flag). Spy and assert not called instead. Don’t get fooled by false negatives.
Apply:
- 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');
+ wrapper.unmount();
+ expect(removeSpy.called).toBe(false);📝 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 documentMock = sandbox.mock(document); | |
| documentMock | |
| .expects('removeEventListener') | |
| .withArgs('contextmenu') | |
| .never(); | |
| documentMock | |
| .expects('removeEventListener') | |
| .withArgs('click') | |
| .never(); | |
| documentMock.expects('removeEventListener').withArgs('contextmenu').never(); | |
| documentMock.expects('removeEventListener').withArgs('click').never(); | |
| // Replace Sinon mock with a spy to catch any calls, regardless of extra args | |
| - const documentMock = sandbox.mock(document); | |
| - documentMock.expects('removeEventListener').withArgs('contextmenu').never(); | |
| - documentMock.expects('removeEventListener').withArgs('click').never(); | |
| - | |
| const removeSpy = sandbox.spy(document, 'removeEventListener'); | |
| wrapper.unmount(); | |
| expect(removeSpy.called).toBe(false); |
| import classNames from 'classnames'; | ||
| import uniqueId from 'lodash/uniqueId'; | ||
| import TetherComponent from 'react-tether'; | ||
| import TetherComponent, { type TetherProps } from 'react-tether'; | ||
|
|
||
| import TetherPosition from '../../common/tether-positions'; | ||
|
|
There was a problem hiding this comment.
Flow syntax in a TS file? I pity the fool!
This .tsx module can’t stomach import { type … }—that’s Flow-only gear. TypeScript (and Babel’s TS loader) will blow up before we even run, leaving RadarAnimation busted. Split the import so the default stays put and the type comes through an import type. Don’t mix your tools, fool!
-import classNames from 'classnames';
-import uniqueId from 'lodash/uniqueId';
-import TetherComponent, { type TetherProps } from 'react-tether';
+import classNames from 'classnames';
+import uniqueId from 'lodash/uniqueId';
+import TetherComponent from 'react-tether';
+import type { TetherProps } from 'react-tether';📝 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.
| import classNames from 'classnames'; | |
| import uniqueId from 'lodash/uniqueId'; | |
| import TetherComponent from 'react-tether'; | |
| import TetherComponent, { type TetherProps } from 'react-tether'; | |
| import TetherPosition from '../../common/tether-positions'; | |
| import classNames from 'classnames'; | |
| import uniqueId from 'lodash/uniqueId'; | |
| import TetherComponent from 'react-tether'; | |
| import type { TetherProps } from 'react-tether'; | |
| import TetherPosition from '../../common/tether-positions'; |
🤖 Prompt for AI Agents
In src/components/radar/RadarAnimation.tsx around lines 2 to 7, the import uses
Flow-only syntax "import TetherComponent, { type TetherProps } from
'react-tether';" which breaks TypeScript; change it to two imports: a normal
default import for TetherComponent and a TypeScript-only "import type {
TetherProps } from 'react-tether';" so the runtime import stays the same and the
type is imported using TS's import type syntax.
| const wrapper = getWrapper({ item }); | ||
|
|
||
| wrapper.find(AnnotationActivityMenu).prop('onDelete')(); | ||
| React.act(() => { | ||
| wrapper.find(AnnotationActivityMenu).prop('onDelete')(); | ||
| }); | ||
|
|
||
| expect(wrapper.exists(DeleteConfirmation)); |
There was a problem hiding this comment.
Add wrapper.update() after act, sucka!
Right now the state flip happens inside React.act, but Enzyme never refreshes, so the tree can stay stale and the DeleteConfirmation assertion lies. Drop in wrapper.update() right after that act block so the test proves the menu really toggled. I pity the fool who trusts a dusty wrapper.
React.act(() => {
wrapper.find(AnnotationActivityMenu).prop('onDelete')();
});
+ wrapper.update();
expect(wrapper.exists(DeleteConfirmation));📝 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 wrapper = getWrapper({ item }); | |
| wrapper.find(AnnotationActivityMenu).prop('onDelete')(); | |
| React.act(() => { | |
| wrapper.find(AnnotationActivityMenu).prop('onDelete')(); | |
| }); | |
| expect(wrapper.exists(DeleteConfirmation)); | |
| const wrapper = getWrapper({ item }); | |
| React.act(() => { | |
| wrapper.find(AnnotationActivityMenu).prop('onDelete')(); | |
| }); | |
| wrapper.update(); | |
| expect(wrapper.exists(DeleteConfirmation)); |
🤖 Prompt for AI Agents
In
src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js
around lines 239 to 245, the Enzyme wrapper isn't refreshed after the React.act
that triggers the delete flow, so the tree can remain stale; add a call to
wrapper.update() immediately after the React.act() block to force Enzyme to
re-render the wrapper before the DeleteConfirmation assertion so the test
observes the updated component tree.
| <TetherComponent | ||
| {...tetherProps} | ||
| renderTarget={ref => ( | ||
| <div ref={ref} style={{ display: 'inline-block' }}> | ||
| {isMenuVisible && ( | ||
| <AnnotationActivityMenu | ||
| canDelete={canDelete} | ||
| canEdit={canEdit} | ||
| canResolve={canResolve} | ||
| className="bcs-AnnotationActivity-menu" | ||
| id={id} | ||
| isDisabled={isConfirmingDelete} | ||
| status={status} | ||
| onDelete={handleDelete} | ||
| onEdit={handleEdit} | ||
| onMenuClose={handleMenuClose} | ||
| onMenuOpen={handleMenuOpen} | ||
| onStatusChange={handleStatusChange} | ||
| /> | ||
| )} | ||
| </div> | ||
| )} | ||
| </TetherComponent> | ||
| 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; | ||
| }} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Fix overlay class application and remove redundant check, fool!
className in tetherProps won’t style the rendered element here. Apply the class to the wrapper in renderElement (or use classes.element). Also, the inner isConfirmingDelete && is redundant since you already guard above.
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;
+ return isConfirmingDelete ? (
+ <div ref={ref} className="bcs-AnnotationActivity-deleteConfirmationModal">
+ <DeleteConfirmation
+ data-resin-component={ACTIVITY_TARGETS.ANNOTATION_OPTIONS}
+ isOpen={isConfirmingDelete}
+ message={messages.annotationActivityDeletePrompt}
+ onDeleteCancel={handleDeleteCancel}
+ onDeleteConfirm={handleDeleteConfirm}
+ />
+ </div>
+ ) : null;
}}Optionally clean up tetherProps to avoid confusion:
const tetherProps = {
attachment: 'top right',
- className: 'bcs-AnnotationActivity-deleteConfirmationModal',
constraints: [{ to: 'scrollParent', attachment: 'together' }],
targetAttachment: 'bottom right',
};Or align with Flyout and use Tether’s classes API:
const tetherProps = {
attachment: 'top right',
- className: 'bcs-AnnotationActivity-deleteConfirmationModal',
+ classes: { element: 'bcs-AnnotationActivity-deleteConfirmationModal' },
constraints: [{ to: 'scrollParent', attachment: 'together' }],
targetAttachment: 'bottom right',
};If you’re unsure whether className is honored by react-tether v3, verify against docs.
🌐 Web query:
In react-tether v3, should styling be applied via the element's own className in renderElement, or via the Tether `classes.element` option? Does TetherComponent support a top-level `className` prop that affects the positioned element?
💡 Result:
Short answer:
- You can style the positioned element either by adding className/style on the element you return from renderElement (recommended for component-specific styling) or by using Tether's classes.element option if you want to change the default tether classes globally/semantically. [1][2]
- react-tether (TetherComponent) also accepts a top-level className/id/style for the wrapper tether element (added in v0.5.0), so a className prop on the component will affect that tether wrapper. [2][3]
Sources:
- Tether docs (classes.element option). [1]
- react-tether README (renderElement, Tether options) and repo. [2]
- react-tether CHANGELOG (added top-level id/className/style in v0.5.0). [3]
Apply modal class in renderElement and drop redundant guard, fool!
- Move
bcs-AnnotationActivity-deleteConfirmationModalfromtetherPropsinto the<div ref={ref}>inrenderElement. - Remove the inner
isConfirmingDelete &&sincerenderElementis already gated by that condition.
🤖 Prompt for AI Agents
In src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js
around lines 201 to 238, move the modal class
bcs-AnnotationActivity-deleteConfirmationModal out of tetherProps and apply it
to the <div ref={ref}> returned by renderElement, and remove the redundant inner
"isConfirmingDelete &&" guard (since renderElement is already conditionally
rendered when isConfirmingDelete is true) so that DeleteConfirmation is rendered
directly inside that div with the modal class.
| const wrapper = mount( | ||
| <AppActivity | ||
| isPending={false} | ||
| onDelete={jest.fn()} | ||
| {...fakeAppActivity} | ||
| currentUser={{ ...fakeUser }} | ||
| permissions={{ can_delete: false }} | ||
| />, | ||
| ); | ||
|
|
||
| expect(wrapper.find('button.bdl-Media-menu').exists()).toBe(true); | ||
| }); | ||
|
|
||
| test('should show the overflow menu if a different user, with the correct permissions', () => { | ||
| const wrapper = render({ | ||
| currentUser: { | ||
| id: 'someone_else', | ||
| }, | ||
| permissions: { | ||
| can_delete: true, | ||
| }, | ||
| }); | ||
|
|
||
| expect(wrapper.exists(Media.Menu)).toBe(true); | ||
| const wrapper = mount( | ||
| <AppActivity | ||
| isPending={false} | ||
| onDelete={jest.fn()} | ||
| {...fakeAppActivity} | ||
| currentUser={{ id: 'someone_else' }} | ||
| permissions={{ can_delete: true }} | ||
| />, | ||
| ); | ||
|
|
||
| expect(wrapper.find('button.bdl-Media-menu').exists()).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Wrap these AppActivity mounts with an IntlProvider, fool.
AppActivity’s default export is wrapped with injectIntl, so full DOM mounting it without an <IntlProvider> blows up with “Could not find required intl object”. These new tests will crash before assertions even run. Drop the bare mount() calls and funnel them through a helper that provides the intl context first.
Apply this fix:
@@
-import { shallow, mount } from 'enzyme';
+import { shallow, mount } from 'enzyme';
+import { IntlProvider } from 'react-intl';
@@
- test('should show the overflow menu if the current user is the one who made the activity', () => {
- const wrapper = mount(
+ const mountWithIntl = (node: React.ReactElement) =>
+ mount(<IntlProvider locale="en">{node}</IntlProvider>);
+
+ test('should show the overflow menu if the current user is the one who made the activity', () => {
+ const wrapper = mountWithIntl(
<AppActivity
@@
- test('should show the overflow menu if a different user, with the correct permissions', () => {
- const wrapper = mount(
+ test('should show the overflow menu if a different user, with the correct permissions', () => {
+ const wrapper = mountWithIntl(
<AppActivityCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/elements/content-sidebar/activity-feed/app-activity/__tests__/AppActivity.test.js
around lines 77 to 102, the tests mount the injectIntl-wrapped AppActivity
directly causing "Could not find required `intl` object" errors; wrap each mount
in an IntlProvider (or use the existing test helper that supplies intl context)
so the injected intl prop is available, e.g., replace direct mount(...) calls
with mounting through the helper or with an <IntlProvider locale="en"> wrapper
around the component and props used in these two tests.
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.
2 participants
Summary by CodeRabbit
Chores
Refactor
Public API
Tests
Style