MM-69835 Remove all usage of findDOMNode - #11
Conversation
I removed the container prop from Modal, but we don't seem to use that ourselves so it's probably fine to do. It was marked as private anyway.
This used to come from react-overlays, but it's not part of the newer version.
hmhealey
changed the title
There was a problem hiding this comment.
The current versions of react-overlays and react-transition-group both use findDOMNode, so I've had to update them. In some cases, they still might try to call it if we pass a React element as a prop instead of an HTMLElement, but my hope is that we only ever use them through react-bootstrap so that won't happen.
The changes here are just porting the previous patch over to the new version of react-overlays. Note that this patch doesn't actually affect the code used at runtime, and the web app is going to actually have its own patch for this library
| return <div {...props} className={classNames(className, classes)} />; | ||
| return ( | ||
| <div | ||
| ref={this.containerRef} |
There was a problem hiding this comment.
Some of these are straightforward because there's already a DOM node that we can attach a ref to instead of using findDOMNode
| const handleExit = createChainedFunction(this.handleExit, onExit); | ||
| const handleExiting = createChainedFunction(this.handleExiting, onExiting); | ||
|
|
||
| const ref = makeMergedRef([this.childRef, getElementRef(children)]); |
There was a problem hiding this comment.
This is a case where we need to inject a ref and hope that it returns a DOM node. I think this may cause an error if Transition doesn't forward its ref to an HTML element, but I don't think we pas anything as Transition other than Fade and Collapse, both of which now forward refs
|
|
||
| focus() { | ||
| const toggle = ReactDOM.findDOMNode(this.toggle); | ||
| const toggle = this.containerRef.current.querySelector( |
There was a problem hiding this comment.
In this case, I didn't think we can pass a ref around for this, so I went with searching the DOM and hoping that the a11y for the toggle button is set up correctly. We use this component in 1 place in the web app (which has tests thankfully) and one or two plugins, so I'm hoping this doesn't break
| } | ||
| } | ||
|
|
||
| function RootCloseWrapper({ |
There was a problem hiding this comment.
The new version of react-overlays doesn't have its own RootCloseWrapper, and I think it's too much work to migrate this to a functional component
| function DialogTransition(props) { | ||
| return <Fade {...props} timeout={Modal.TRANSITION_DURATION} />; | ||
| } | ||
| const DialogTransition = React.forwardRef((props, ref) => ( |
There was a problem hiding this comment.
These forwardRefs aren't actually needed once we're using React 19, but I wanted to make sure this all works with React 18 to ease in the upgrade
| prefix(props, 'backdrop'), | ||
| backdropClassName, | ||
| inClassName | ||
| renderBackdrop={backdropProps => ( |
There was a problem hiding this comment.
There's a change in the API for the react-overlays Modal because:
- The
backdropClassNameprop was replaced byrenderBackdrop - Instead of passing a child that gets cloned to inject props, there's now a
renderDialogprop.
I think I've got them both working the same now since I didn't want to change the API of our React Bootstrap components at all
| bsClass: PropTypes.string, | ||
| bsSize: PropTypes.oneOf(SIZES) |
There was a problem hiding this comment.
These should've been in #12, so just ignore them for now
| {child} | ||
| <BaseOverlay {...props} target={null} transition={transition}> | ||
| {({ props: overlayProps }) => ( | ||
| <div ref={overlayProps.ref} style={{ display: 'content' }}> |
There was a problem hiding this comment.
In this case, I didn't have an option for attaching a ref to the arbitrary child or using a11y attributes, so I instead opted to add an extra div and just hope that it can properly match the size of its child by using display: content
|
@coderabbitai Review this |
|
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe change updates overlay dependencies and Escape handling. It replaces ChangesReact ref and overlay modernization
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
package.jsonParsing error: Unexpected token, expected ";" 1 | {
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Collapse.js (1)
201-216: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the DOM node in
Collapsetransition callbacks.With
react-transition-group@4.4.5,nodeRefomits the DOM node. Enter callbacks receiveisAppearing, exit callbacks receiveundefined, andaddEndListenerreceives onlydone. Wrap all exposed callbacks to passthis.childRef.current, and add regression tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Collapse.js` around lines 201 - 216, Update the callback wrappers in Collapse’s transition setup to prepend this.childRef.current to onEnter, onEntering, onEntered, onExit, onExiting, and addEndListener arguments, preserving isAppearing for enter callbacks and undefined for exit callbacks. Ensure consumers receive the DOM node despite nodeRef, and add regression coverage for these callback signatures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 129: Update TabPane’s caller-supplied transition rendering to create and
pass a DOM nodeRef to the transition, and attach that ref to the pane element so
custom transitions avoid findDOMNode. Preserve the existing Fade, Collapse, and
modal transition behavior, while requiring custom transition components to
consume the provided nodeRef.
In `@src/Fade.js`:
- Around line 72-101: Update the Fade transition callbacks around the Transition
usage to restore the DOM-node first argument omitted when nodeRef is provided.
Wrap onEnter, onEntering, onEntered, onExit, onExiting, onExited, and
addEndListener so each invokes the supplied callback with childRef.current first
while preserving remaining arguments and existing behavior, including
Modal/DialogTransition forwarding. Add regression coverage for enter, exit, and
addEndListener callbacks.
In `@src/Modal.js`:
- Around line 279-294: Restore compatibility for dialogComponentClass values
that do not forward refs by preserving the previous wrapper behavior around the
Dialog render path. Ensure this._modal.dialog consistently references the
expected DOM dialog element so updateStyle() works when the modal enters, while
retaining support for ref-forwarding components and existing styling props.
In `@src/Overlay.js`:
- Around line 91-96: Update the BaseOverlay invocation and rendered overlay
element in Overlay: remove target={null} so triggerRef is passed through, and
spread the generated overlayProps onto the div alongside its ref and existing
style. Preserve the child rendering and current display style.
In `@src/OverlayTrigger.js`:
- Around line 297-299: Update the trigger rendering in OverlayTrigger so
triggerRef attaches to the actual geometry-bearing trigger DOM node via ref
forwarding or the established render-prop contract, rather than relying on
invalid display:'content'. If arbitrary children require a wrapper, give it an
explicit measurable inline layout contract and preserve trigger positioning; add
a regression test covering inline-trigger geometry.
In `@src/utils/mergeRefs.js`:
- Around line 10-20: Update makeMergedRef to retain cleanup functions returned
by callback refs, and return a composed cleanup that clears object refs, invokes
each returned cleanup, and calls ref(null) only for callbacks without cleanup.
Ensure useMergedRef uses this updated behavior without discarding the returned
cleanup.
---
Outside diff comments:
In `@src/Collapse.js`:
- Around line 201-216: Update the callback wrappers in Collapse’s transition
setup to prepend this.childRef.current to onEnter, onEntering, onEntered,
onExit, onExiting, and addEndListener arguments, preserving isAppearing for
enter callbacks and undefined for exit callbacks. Ensure consumers receive the
DOM node despite nodeRef, and add regression coverage for these callback
signatures.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bad325a-fb9c-481e-a107-91abbddd8205
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
package.jsonpatches/react-overlays+0.9.3.patchpatches/react-overlays+5.2.1.patchsrc/CarouselItem.jssrc/Collapse.jssrc/Dropdown.jssrc/DropdownMenu.jssrc/Fade.jssrc/Modal.jssrc/ModalDialog.jssrc/Nav.jssrc/Overlay.jssrc/OverlayTrigger.jssrc/utils/mergeRefs.jstest/DropdownSpec.jstest/FadeSpec.jstest/ModalSpec.jstest/OverlayTriggerSpec.jstest/index.js
💤 Files with no reviewable changes (3)
- test/DropdownSpec.js
- test/index.js
- patches/react-overlays+0.9.3.patch
| "react-overlays": "^5.2.0", | ||
| "react-prop-types": "^0.4.0", | ||
| "react-transition-group": "^2.0.0", | ||
| "react-transition-group": "^4.4.0", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 6 'findDOMNode|nodeRef|<Transition\b|<CSSTransition\b' src testRepository: mattermost/react-bootstrap
Length of output: 4347
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- transition-related source files ---'
rg -n -C 12 'findDOMNode|nodeRef|<Transition\b|<CSSTransition\b|Transition\s*=' src test
printf '%s\n' '--- TabPane implementation ---'
sed -n '180,275p' src/TabPane.js
printf '%s\n' '--- transition component definitions and exports ---'
rg -n -C 8 'function .*Transition|class .*Transition|const .*Transition|export .*Transition|react-transition-group' src
printf '%s\n' '--- package dependency and repository metadata ---'
sed -n '115,140p' package.json
git ls-files 'src/**' 'test/**' | sortRepository: mattermost/react-bootstrap
Length of output: 24171
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- all animation and transition call sites ---'
rg -n -C 8 'animation|transition|backdropTransition|Transition|CSSTransition' src \
-g '*.js'
printf '%s\n' '--- transition-related imports and JSX ---'
rg -n -C 5 'react-transition-group|<Transition\b|<CSSTransition\b|<Fade\b|<Collapse\b' src \
-g '*.js'
printf '%s\n' '--- dynamic Transition verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
for path in sorted(Path("src").rglob("*.js")):
text = path.read_text()
for match in re.finditer(r"<Transition\b|<CSSTransition\b", text):
line = text.count("\n", 0, match.start()) + 1
end = text.find(">", match.start())
opening = text[match.start():end + 1] if end != -1 else text[match.start():]
print(f"{path}:{line}: {opening.replace(chr(10), ' ')}")
for match in re.finditer(r"const\s+Transition\s*=\s*([^;]+);", text):
line = text.count("\n", 0, match.start()) + 1
print(f"{path}:{line}: dynamic Transition = {match.group(1).strip()}")
PYRepository: mattermost/react-bootstrap
Length of output: 43757
Pass a DOM nodeRef through TabPane transitions.
Fade, Collapse, and modal transitions supply nodeRef, but src/TabPane.js:196,234 renders a caller-supplied transition without it. A react-transition-group Transition can therefore call findDOMNode under React 19. Pass a ref to the pane and require custom transition components to use it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 129, Update TabPane’s caller-supplied transition
rendering to create and pass a DOM nodeRef to the transition, and attach that
ref to the pane element so custom transitions avoid findDOMNode. Preserve the
existing Fade, Collapse, and modal transition behavior, while requiring custom
transition components to consume the provided nodeRef.
Source: MCP tools
| { | ||
| className, | ||
| children, | ||
| in: inProp = false, | ||
| timeout = 300, | ||
| mountOnEnter = false, | ||
| unmountOnExit = false, | ||
| appear = false, | ||
| ...props | ||
| }, | ||
| ref | ||
| ) => { | ||
| const childRef = React.useRef(null); | ||
|
|
||
| const setChildRef = useMergedRef([childRef, getElementRef(children), ref]); | ||
|
|
||
| return ( | ||
| <Transition {...props}> | ||
| <Transition | ||
| {...props} | ||
| in={inProp} | ||
| timeout={timeout} | ||
| mountOnEnter={mountOnEnter} | ||
| unmountOnExit={unmountOnExit} | ||
| appear={appear} | ||
| nodeRef={childRef} | ||
| > | ||
| {(status, innerProps) => | ||
| React.cloneElement(children, { | ||
| ...innerProps, | ||
| ref: setChildRef, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate consumers that expect transition callback arguments.
rg -n -C3 -g '*.js' \
'(<Fade\b|addEndListener|onEnter|onEntering|onEntered|onExit|onExiting|onExited)' \
src testRepository: mattermost/react-bootstrap
Length of output: 20160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dependency declarations ---'
rg -n -C2 'react-transition-group' package.json package-lock.json yarn.lock 2>/dev/null || true
printf '%s\n' '--- Fade implementation ---'
cat -n src/Fade.js | sed -n '1,150p'
printf '%s\n' '--- Collapse implementation ---'
cat -n src/Collapse.js | sed -n '180,245p'
printf '%s\n' '--- transition callback tests and usages ---'
rg -n -C4 -g '*.js' \
'addEndListener|onEnter|onEntering|onEntered|onExit|onExiting|onExited' \
test/FadeSpec.js test/ModalSpec.js src/Modal.js src/Collapse.js test 2>/dev/null | sed -n '1,260p'Repository: mattermost/react-bootstrap
Length of output: 17594
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
curl -fsSL \
https://raw.githubusercontent.com/reactjs/react-transition-group/v4.4.5/src/Transition.js \
-o "$tmpdir/Transition.js"
printf '%s\n' '--- react-transition-group 4.4.5 callback implementation ---'
rg -n -C12 \
'nodeRef|onEnter|onEntering|onEntered|onExit|onExiting|onExited|addEndListener' \
"$tmpdir/Transition.js" | sed -n '1,300p'
printf '%s\n' '--- repository callback documentation ---'
rg -n -C5 \
'Callback fired|onEnter|onEntering|onEntered|onExit|onExiting|onExited|addEndListener' \
README.md docs src test package.json 2>/dev/null | sed -n '1,320p'Repository: mattermost/react-bootstrap
Length of output: 23094
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
def react_transition_group_args(node_ref, node, appearing):
if node_ref:
return {
"onEnter": (appearing,),
"onEntering": (appearing,),
"onEntered": (appearing,),
"onExit": (),
"onExiting": (),
"onExited": (),
"addEndListener": ("done",),
}
return {
"onEnter": (node, appearing),
"onEntering": (node, appearing),
"onEntered": (node, appearing),
"onExit": (node,),
"onExiting": (node,),
"onExited": (node,),
"addEndListener": (node, "done"),
}
def fade_compatibility_args(callback, callback_args, node):
if callback == "addEndListener":
return (node, callback_args[0])
if callback in {"onEnter", "onEntering", "onEntered"}:
return (node, callback_args[0])
return (node,)
node = "<childRef.current>"
actual = react_transition_group_args(True, node, True)
expected = react_transition_group_args(False, node, True)
for callback in expected:
adapted = fade_compatibility_args(callback, actual[callback], node)
assert adapted == expected[callback], (
callback,
adapted,
expected[callback],
)
print("All Fade callback wrappers restore the DOM node as the first argument.")Repository: mattermost/react-bootstrap
Length of output: 322
Restore the DOM-node callback argument.
nodeRef causes react-transition-group@4.4.5 to omit the DOM node from transition callbacks. Wrap onEnter, onEntering, onEntered, onExit, onExiting, onExited, and addEndListener before passing them to Transition. Pass childRef.current as the first argument. This preserves Fade callbacks, including callbacks forwarded by Modal through DialogTransition. Add enter, exit, and addEndListener regression tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Fade.js` around lines 72 - 101, Update the Fade transition callbacks
around the Transition usage to restore the DOM-node first argument omitted when
nodeRef is provided. Wrap onEnter, onEntering, onEntered, onExit, onExiting,
onExited, and addEndListener so each invokes the supplied callback with
childRef.current first while preserving remaining arguments and existing
behavior, including Modal/DialogTransition forwarding. Add regression coverage
for enter, exit, and addEndListener callbacks.
| renderDialog={renderProps => ( | ||
| <Dialog | ||
| {...dialogProps} | ||
| {...renderProps} | ||
| style={{ ...renderProps.style, ...this.state.style, ...style }} | ||
| className={classNames( | ||
| renderProps.className, | ||
| className, | ||
| inClassName | ||
| )} | ||
| onClick={backdrop === true ? this.handleDialogClick : null} | ||
| handleDialogMouseDown={this.handleDialogMouseDown} | ||
| > | ||
| {children} | ||
| </Dialog> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find custom dialog implementations and their Modal call sites.
rg -n -C4 -g '*.js' \
'\bdialogComponentClass\b|<ModalDialog\b|React\.forwardRef' \
src testRepository: mattermost/react-bootstrap
Length of output: 3539
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Modal.js ---'
sed -n '1,340p' src/Modal.js
printf '%s\n' '--- ModalDialog.js ---'
sed -n '1,180p' src/ModalDialog.js
printf '%s\n' '--- dependency declarations ---'
rg -n -C3 'react-overlays|react-bootstrap|dialogComponentClass' package.json package-lock.json yarn.lock 2>/dev/null || true
printf '%s\n' '--- BaseModal references ---'
rg -n -C5 'BaseModal|renderDialog|updateStyle|_modal' src testRepository: mattermost/react-bootstrap
Length of output: 21999
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for version in 5.2.0 5.2.1; do
printf '%s\n' "--- react-overlays ${version} Modal.js ---"
curl -fsSL "https://unpkg.com/react-overlays@${version}/esm/Modal.js" |
sed -n '1,280p'
done
printf '%s\n' '--- React peer versions ---'
sed -n '/"peerDependencies"/,/^[[:space:]]*},/p' package.json
printf '%s\n' '--- Modal history summary ---'
git log --oneline --all -- src/Modal.js test/ModalSpec.js | head -30Repository: mattermost/react-bootstrap
Length of output: 21134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- renderDialog change ---'
git show --format=fuller --stat 8a8353a -- src/Modal.js test/ModalSpec.js
git show --format= --unified=80 8a8353a -- src/Modal.js test/ModalSpec.js | sed -n '1,260p'
printf '%s\n' '--- preceding Modal implementation ---'
parent="$(git rev-parse 8a8353a^)"
git show "${parent}:src/Modal.js" | sed -n '210,330p'
printf '%s\n' '--- current commit and recent change ---'
git log -1 --format='%H%n%P%n%s' -- src/Modal.js
git show --format= --unified=40 2dd3aae -- src/Modal.js test/ModalSpec.js | sed -n '1,220p'
printf '%s\n' '--- relevant dependency helper source ---'
curl -fsSL https://unpkg.com/dom-helpers@5.2.1/esm/ownerDocument.js
curl -fsSL https://unpkg.com/dom-helpers@5.2.1/esm/contains.jsRepository: mattermost/react-bootstrap
Length of output: 14323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dialogComponentClass documentation and examples ---'
rg -n -C6 'dialogComponentClass|custom.*dialog|custom dialog' . \
-g '!node_modules' -g '!dist' -g '!build' -g '!coverage'
printf '%s\n' '--- current custom-dialog test ---'
sed -n '220,250p' test/ModalSpec.js
printf '%s\n' '--- BaseModal renderDialog contract ---'
curl -fsSL https://unpkg.com/react-overlays@5.2.1/esm/Modal.d.ts |
rg -n -C8 'renderDialog|dialog'Repository: mattermost/react-bootstrap
Length of output: 50383
Preserve dialogComponentClass compatibility.
Custom dialog components that do not forward refs now leave this._modal.dialog null or set it to a component instance. updateStyle() then fails when the modal enters. Restore the previous wrapper behaviour, or document this breaking change and add migration guidance. The updated test only covers ref-forwarding components.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Modal.js` around lines 279 - 294, Restore compatibility for
dialogComponentClass values that do not forward refs by preserving the previous
wrapper behavior around the Dialog render path. Ensure this._modal.dialog
consistently references the expected DOM dialog element so updateStyle() works
when the modal enters, while retaining support for ref-forwarding components and
existing styling props.
| <BaseOverlay {...props} target={null} transition={transition}> | ||
| {({ props: overlayProps }) => ( | ||
| <div ref={overlayProps.ref} style={{ display: 'content' }}> | ||
| {child} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- src/Overlay.js ---'
cat -n src/Overlay.js | sed -n '1,150p'
printf '%s\n' '--- src/OverlayTrigger.js ---'
cat -n src/OverlayTrigger.js | sed -n '180,235p'
printf '%s\n' '--- react-overlays references ---'
rg -n --hidden -S 'BaseOverlay|react-overlays|overlayProps|target=\{null\}|display: .content.' . \
-g '!node_modules' -g '!dist' -g '!build'Repository: mattermost/react-bootstrap
Length of output: 9218
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- bundled local Overlay implementation ---'
cat -n lib/Overlay.js | sed -n '1,145p'
printf '%s\n' '--- AMD Overlay implementation ---'
sed -n '14360,14520p' amd/react-bootstrap.js
printf '%s\n' '--- local references to overlay render props ---'
rg -n -C 8 'function Overlay|render\\(\\).*target|target.*popper|overlayProps|style:.*position|attributes' \
amd/react-bootstrap.js lib es src patches \
-g '*.js' -g '*.patch' | sed -n '1,260p'Repository: mattermost/react-bootstrap
Length of output: 23387
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- bundled react-overlays Overlay implementation ---'
cat -n amd/react-bootstrap.js | sed -n '6520,6725p'
printf '%s\n' '--- dependency metadata ---'
sed -n '118,134p' package.json
sed -n '5368,5384p' yarn.lockRepository: mattermost/react-bootstrap
Length of output: 8881
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from urllib.request import urlopen
urls = [
"https://unpkg.com/react-overlays@5.2.1/esm/Overlay.js",
"https://unpkg.com/react-overlays@5.2.1/esm/Position.js",
]
for url in urls:
print(f"--- {url} ---")
with urlopen(url, timeout=15) as response:
text = response.read().decode()
for number, line in enumerate(text.splitlines(), 1):
print(f"{number:4}: {line}")
PYRepository: mattermost/react-bootstrap
Length of output: 2512
🏁 Script executed:
#!/bin/bash
set -eu
for path in esm/Overlay.js esm/Position.js; do
url="https://unpkg.com/react-overlays@5.2.1/$path"
printf '%s\n' "--- $url ---"
curl -ksSfL "$url" | nl -ba
doneRepository: mattermost/react-bootstrap
Length of output: 366
🌐 Web query:
react-overlays 5.2.1 esm Overlay.js usePopper render prop props target
💡 Result:
In react-overlays 5.2.1, the Overlay component provides functionality for positioning elements relative to a target using Popper.js [1][2]. The Overlay component uses a render prop (as its children) to allow full control over rendering the overlay element [1][3]. This render prop function receives an object containing positioning information and props that must be spread onto the overlay element to ensure correct positioning and styling [1][3]. The render prop signature for the Overlay children is a function that returns a React element [1]: ({ show, placement, update, forceUpdate, props, arrowProps }) => React.Element Key elements of the render prop: - props: An object containing essential attributes (e.g., ref, style) that must be applied to the overlay element to connect it to the Popper.js positioning engine [1][3]. - arrowProps: Optional props for positioning an arrow element if used [1][3]. - show, placement: State information about the current visibility and positioning of the overlay [1]. The target prop is required to define the reference point for the overlay [1][4]. It accepts a DOM Element, a ref to an element, or a function that returns either [1]. Example usage: import { Overlay } from 'react-overlays'; import { useRef } from 'react'; function Example({ show }) { const target = useRef(null); return ( <>
Citations:
- 1: https://react-bootstrap.github.io/react-overlays/api/Overlay/
- 2: https://www.jsdocs.io/package/react-overlays
- 3: https://react-bootstrap.netlify.app/docs/components/overlays/
- 4: Passing target element to Overlay component react-bootstrap/react-bootstrap#3007
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
overlay = Path("src/Overlay.js").read_text()
trigger = Path("src/OverlayTrigger.js").read_text()
base_call = re.search(
r"<BaseOverlay\s+\{\.\.\.props\}([^>]*)>", overlay, re.S
)
div = re.search(
r"<div([^>]*)>\s*\{child\}\s*</div>", overlay, re.S
)
trigger_call = re.search(
r"<Overlay\s*\n\s*\{\.\.\.props\}.*?\n\s*target=\{this\.triggerRef\}",
trigger,
re.S,
)
print("OverlayTrigger passes triggerRef:", bool(trigger_call))
print("BaseOverlay props:", " ".join(base_call.group(1).split()) if base_call else "<not found>")
print("Overlay div props:", " ".join(div.group(1).split()) if div else "<not found>")
if not base_call or "target={null}" not in base_call.group(1):
raise SystemExit("expected target={null} override was not found")
if not div or "ref={overlayProps.ref}" not in div.group(1):
raise SystemExit("expected ref forwarding was not found")
if "overlayProps" in div.group(1):
raise SystemExit("unexpected full overlayProps spread")
PYRepository: mattermost/react-bootstrap
Length of output: 369
Restore the target and generated overlay props.
Remove target={null} so triggerRef reaches BaseOverlay. Spread overlayProps onto the overlay element so Popper-generated positioning styles and attributes are applied.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Overlay.js` around lines 91 - 96, Update the BaseOverlay invocation and
rendered overlay element in Overlay: remove target={null} so triggerRef is
passed through, and spread the generated overlayProps onto the div alongside its
ref and existing style. Preserve the child rendering and current display style.
| <div ref={this.triggerRef} style={{ display: 'content' }}> | ||
| {cloneElement(child, triggerProps)} | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
printf '%s\n' '--- files ---'
git ls-files 'src/OverlayTrigger.js' 'src/*' | sed -n '1,120p'
printf '%s\n' '--- OverlayTrigger outline ---'
ast-grep outline src/OverlayTrigger.js --view compact 2>/dev/null || true
printf '%s\n' '--- target and display usages ---'
rg -n -C 4 "triggerRef|display: ['\"]content|Overlay\.target|Overlay" src/OverlayTrigger.js src -g '*.js' | sed -n '1,240p'Repository: mattermost/react-bootstrap
Length of output: 12908
🏁 Script executed:
printf '%s\n' '--- OverlayTrigger implementation ---'
sed -n '1,330p' src/OverlayTrigger.js
printf '%s\n' '--- Overlay implementation ---'
sed -n '1,130p' src/Overlay.js
printf '%s\n' '--- related tests and usages ---'
git ls-files | rg '(^|/)(test|tests|__tests__)/|OverlayTrigger|Dropdown\.js$' | sed -n '1,160p'
rg -n -C 5 "<OverlayTrigger|OverlayTrigger|display: ['\"]contents['\"]|display: ['\"]content['\"]" test tests src README.md docs 2>/dev/null | sed -n '1,280p'
printf '%s\n' '--- package versions ---'
sed -n '1,180p' package.jsonRepository: mattermost/react-bootstrap
Length of output: 30850
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
import re
trigger = Path('src/OverlayTrigger.js').read_text()
overlay = Path('src/Overlay.js').read_text()
ref_target = bool(re.search(r'target=\{this\.triggerRef\}', trigger))
wrapper = re.search(
r'<div ref=\{this\.triggerRef\} style=\{\{ display: [\'"]([^\'"]+)[\'"] \}\}>',
trigger,
)
child_clone = bool(re.search(r'cloneElement\(child, triggerProps\)', trigger))
print({
'overlay_target_uses_trigger_ref': ref_target,
'trigger_wrapper_display_value': wrapper.group(1) if wrapper else None,
'child_is_cloned_with_trigger_props': child_clone,
'overlay_wrapper_display_value': re.search(
r'<div ref=\{overlayProps\.ref\} style=\{\{ display: [\'"]([^\'"]+)[\'"] \}\}>',
overlay,
).group(1),
})
PY
printf '%s\n' '--- complete OverlayTrigger tests ---'
cat -n test/OverlayTriggerSpec.js | sed -n '1,360p'
printf '%s\n' '--- exact dependency lock entries ---'
rg -n -C 3 '"(react|react-dom|react-overlays)"' yarn.lock package-lock.json 2>/dev/null | sed -n '1,180p'Repository: mattermost/react-bootstrap
Length of output: 11739
🌐 Web query:
CSS Display Module Level 3 invalid display value content display contents principal box getBoundingClientRect
💡 Result:
Under the CSS Display Module Level 3 specification, elements with display: contents do not generate a principal box [1][2][3]. Instead, the element is effectively replaced by its contents (children and pseudo-elements) in the box tree for the purposes of layout and box generation [1][4][3]. Because the element itself does not generate a box, it has no dimensions, position, or associated bounding box [5][6][7]. Consequently, when you call getBoundingClientRect on an element with display: contents, browsers return a DOMRect with all properties (x, y, width, height, top, etc.) set to zero [5][6][7]. This behavior is a direct consequence of the element having no box in the rendering tree [5]. While there are ongoing discussions within the W3C CSS Working Group regarding potential changes to this behavior (e.g., whether getClientRects should return a flattened list of its children's rectangles) [8][9], current browser implementations consistently follow the existing specification where the element is "invisible" to the layout engine, resulting in zero-sized measurements [5][6][7]. Developers relying on bounding boxes for positioning (such as in tooltip or popover libraries) must typically work around this by measuring the element's children instead [5][6][7]. Note that for replaced elements (such as or ) or other elements whose rendering is not entirely controlled by CSS, display: contents may compute to display: none or otherwise be treated as invalid, causing the element to be removed from the box tree entirely [10][3]. Using display: none is recommended for these elements to ensure predictable behavior [3].
Citations:
- 1: https://drafts.csswg.org/css-display-3/
- 2: https://www.w3.org/TR/2026/CRD-css-display-3-20260605/
- 3: https://drafts.csswg.org/css-display-4/
- 4: https://www.w3.org/TR/2022/CRD-css-display-3-20221118/
- 5: Incorrect positioning when reference has
display: contentsfloating-ui/floating-ui#2403 - 6: https://stackoverflow.com/questions/75454061/getboundingclientrect-from-a-div-with-as-style-display-contents
- 7: Not able to render element which has display:contents; property qq15725/modern-screenshot#83
- 8: [cssom-view]
getClientRects()should return a flattened list of child rects fordisplay: contentsw3c/csswg-drafts#12040 - 9: https://lists.w3.org/Archives/Public/public-css-archive/2025Apr/0127.html
- 10: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/display
🌐 Web query:
MDN CSS display property valid values contents content invalid declaration initial value block div
💡 Result:
The CSS display property determines how an element generates boxes and how its children are laid out [1][2]. Initial Value: The initial value of the display property is inline [3][4][5]. Valid Values: The display property accepts a wide range of keyword values [1][3]. Modern CSS specifications define a multi-keyword syntax that allows for explicit control over an element's outer and inner display types [1][6][4]. For backward compatibility, browsers also support legacy single-keyword values (e.g., block, flex, grid) [1][6]. Commonly used values include: - block: Sets the element as a block-level box [3][2]. - inline: Sets the element as an inline-level box [3][2]. - contents: The element itself generates no box; instead, its children are rendered as if they were direct children of the element's parent [7][2]. - flex: Lays out children using the flexbox model [8][3]. - grid: Lays out children using the grid model [8][3]. - none: The element and its subtree are removed from the box tree and are not rendered [3][2]. Relationship between "block", "contents", and "content": - block: A standard display value that makes an element a block-level container [7][2]. - contents: A unique value that causes an element to be ignored in the box tree, effectively allowing its children to be positioned directly within the parent context [7][2]. - content: "content" is not a valid value for the display property. If used (e.g., display: content;), it is treated as an invalid declaration and ignored by the browser, with the element falling back to its previous or default display value [1][3]. The full specification uses a pair of keywords (an outer type and an inner type) [3][4][9]. For example, display: block flex is equivalent to the shorthand display: flex, where the outer type defaults to block [1][6].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/display
- 2: https://drafts.csswg.org/css-display-3/
- 3: https://www.w3.org/TR/css-display-4/
- 4: https://www.w3.org/TR/css-display-3/
- 5: https://www.w3.org/TR/2026/CRD-css-display-3-20260605/
- 6: https://developer.mozilla.org/en-US/docs/Web/CSS/display?summary=
- 7: https://css-tricks.com/almanac/properties/d/display/
- 8: https://github.com/mdn/content/blob/main/files/en-us/web/css/reference/properties/display/index.md
- 9: https://drafts.csswg.org/css-display-4/
Use a valid, geometry-bearing trigger target.
display: 'content' is invalid, so this div remains a block-level box and can change inline trigger layout. Do not replace it with display: 'contents', because the target would have no measurable box.
Attach triggerRef to the trigger DOM node through ref forwarding or a render-prop contract. If arbitrary children remain supported, use a wrapper with an explicit measurable layout contract. Add an inline-trigger regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/OverlayTrigger.js` around lines 297 - 299, Update the trigger rendering
in OverlayTrigger so triggerRef attaches to the actual geometry-bearing trigger
DOM node via ref forwarding or the established render-prop contract, rather than
relying on invalid display:'content'. If arbitrary children require a wrapper,
give it an explicit measurable inline layout contract and preserve trigger
positioning; add a regression test covering inline-trigger geometry.
| export function makeMergedRef(refs) { | ||
| return el => { | ||
| refs.forEach(ref => { | ||
| if (typeof ref === 'function') { | ||
| ref(el); | ||
| } else if (ref != null) { | ||
| ref.current = el; | ||
| } | ||
| }); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tracked candidate files ---'
git ls-files | rg '(^|/)(mergeRefs\.js|Collapse\.js|Dropdown\.js|Fade\.js|package\.json|.*test.*|.*spec.*)$' | head -200
printf '%s\n' '--- mergeRefs.js ---'
cat -n src/utils/mergeRefs.js
printf '%s\n' '--- consumer excerpts ---'
for f in src/Collapse.js src/Dropdown.js src/Fade.js; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f" | sed -n '70,105p;175,250p;235,275p'
fi
done
printf '%s\n' '--- ref utility usages and package metadata ---'
rg -n --glob '!node_modules' 'makeMergedRef|useMergedRef|getElementRef|ReactDOM|react' src package.json .github 2>/dev/null | head -300Repository: mattermost/react-bootstrap
Length of output: 22683
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- package and test configuration ---'
cat -n package.json | sed -n '40,125p'
printf '%s\n' '--- ref-related tests ---'
rg -n -C 5 'ref|callback|cleanup|Collapse|Fade|Dropdown' test/CollapseSpec.js test/FadeSpec.js test/DropdownSpec.js test/helpers.js test/index.js 2>/dev/null | head -300
printf '%s\n' '--- all source and test references to mergeRefs ---'
rg -n -C 3 'makeMergedRef|useMergedRef|getElementRef' src test es lib
printf '%s\n' '--- repository state ---'
git status --short
git diff --statRepository: mattermost/react-bootstrap
Length of output: 21534
🌐 Web query:
React 19 callback ref cleanup function official documentation callback refs cleanup
💡 Result:
In React 19, you can now return a cleanup function from a ref callback [1][2][3]. This functionality allows you to perform cleanup tasks—such as disconnecting an IntersectionObserver, removing event listeners, or destroying third-party chart instances—directly within the ref callback when the component unmounts [1][4][5][6]. Key details of this feature include: How it works: When the component unmounts, React will execute the cleanup function returned by your ref callback [1][2][4]. If a cleanup function is returned, React will skip the legacy behavior of calling the ref function with null upon unmounting [1][3][6]. Implementation example: <input ref={(ref) => { // Setup logic const observer = new ResizeObserver(...); if (ref) observer.observe(ref); // Return cleanup function return => { observer.disconnect; }; }} /> TypeScript and Implicit Returns: Because returning any value from a ref callback now indicates a cleanup function, TypeScript will reject implicit returns [7][1][3]. You must explicitly wrap your callback in a block and avoid returning non-cleanup values [7][8][3]. For example: // Incorrect (implicit return) <div ref={current => (instance = current)} /> // Correct (explicit return) <div ref={current => { instance = current; }} /> Migration and Compatibility: React plans to deprecate the behavior of calling ref functions with null during unmounting in future versions [1][3]. If your existing code relies on this behavior, you should transition to the new cleanup function pattern [7][1]. Tools such as the no-implicit-ref-callback-return codemod are available to help identify and fix implicit returns that may conflict with this new feature [7][1].
Citations:
- 1: https://react.dev/blog/2024/12/05/react-19
- 2: https://github.com/react/react/blob/refs/heads/main/CHANGELOG.md
- 3: https://blog.saeloun.com/2025/03/24/react-19-ref-as-prop/
- 4: https://loke.dev/blog/react-19-ref-cleanup-callback
- 5: https://www.stanza.dev/courses/react-intermediate/useref/react-ref-callback
- 6: https://tkdodo.eu/blog/ref-callbacks-react-19-and-the-compiler
- 7: https://react.dev/blog/2024/04/25/react-19-upgrade-guide
- 8: https://www.callstack.com/blog/the-complete-developer-guide-to-react-19-part-2-new-enhancements
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class RefObject:
current: object = None
def current_merged(refs, el):
for ref in refs:
if callable(ref):
ref(el)
elif ref is not None:
ref.current = el
def proposed_merged(refs, el):
cleanups = []
for ref in refs:
if callable(ref):
cleanup = ref(el)
cleanups.append(cleanup if callable(cleanup) else (lambda ref=ref: ref(None)))
elif ref is not None:
ref.current = el
cleanups.append(lambda ref=ref: setattr(ref, "current", None))
else:
cleanups.append(None)
active = [cleanup for cleanup in cleanups if cleanup is not None]
return (lambda: [cleanup() for cleanup in active]) if active else None
events = []
def callback(node):
events.append(("attach", node))
return lambda: events.append(("cleanup", node))
obj = RefObject()
cleanup = None
current_merged([callback, obj], "node")
print("current implementation:", events, obj.current, cleanup)
events.clear()
obj = RefObject()
cleanup = proposed_merged([callback, obj], "node")
print("proposed attach:", events, obj.current, callable(cleanup))
cleanup()
print("proposed detach:", events, obj.current)
PYRepository: mattermost/react-bootstrap
Length of output: 331
🌐 Web query:
site:github.com/react-bootstrap/react-bootstrap mergeRefs callback ref cleanup makeMergedRef
💡 Result:
In the context of React libraries like react-bootstrap and @restart/hooks, mergeRefs is a utility function used to combine multiple refs into a single ref callback [1][2][3]. This is necessary when a component needs to maintain its own internal ref to a DOM node while also exposing a ref provided by a parent via forwardRef [1][2]. The mergeRefs utility typically handles cleanup by returning a function or managing a callback ref that updates all provided refs when the component mounts or unmounts [2][3]. When a ref is no longer needed—such as when a component unmounts—the merged ref callback is called with null, which in turn ensures that all original refs are also set to null, preventing memory leaks and stale references [4]. Key points regarding these patterns: 1. Ref Merging: Functions like useMergedRefs (often powered by a mergeRefs utility) are designed to "merge" a user-provided ref (from a parent) and an internal ref (needed for component logic) into a single callback ref that the component applies to the DOM element [2][3]. 2. Cleanup: Because these are callback refs, React automatically invokes them with null during cleanup (unmounting). The mergeRefs implementation ensures this null value is propagated to every individual ref passed into the merger [2][4]. 3. makeMergedRef: While mergeRefs is the common utility name, some older or specific internal implementations use helpers like makeMergedRef to construct the callback function that orchestrates these assignments. For modern React development, ensure you are using the official useMergedRefs hook from @restart/hooks or a similar community-standard library, as they handle these edge cases, including cleanup and proper ref forwarding, consistently across different React versions [1][3].
Citations:
- 1: fix(AbstractNav): allow passed in refs to be properly forwarded react-bootstrap/react-bootstrap#4031
- 2: https://github.com/react-bootstrap/react-bootstrap/blob/v2.4.0/src/Modal.tsx
- 3: react-bootstrap@7030465
- 4: Fix #6991: Clear cached popper references when overlays hide react-bootstrap/react-bootstrap#6995
Preserve callback-ref cleanup functions in makeMergedRef. React 19 callback refs can return cleanup functions, but the merged ref discards them. Return a composed cleanup that clears object refs, invokes returned cleanups, and calls ref(null) only when a callback returns no cleanup. This also affects useMergedRef.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/mergeRefs.js` around lines 10 - 20, Update makeMergedRef to retain
cleanup functions returned by callback refs, and return a composed cleanup that
clears object refs, invokes each returned cleanup, and calls ref(null) only for
callbacks without cleanup. Ensure useMergedRef uses this updated behavior
without discarding the returned cleanup.
Source: MCP tools
1 participant
Summary
The last big change to get this working with React 19 is to remove usage of findDOMNode. This is also the one that's the most likely to be a breaking change because it makes it so that we need to pass HTML elements around or use
querySelectorto get them which I did in a few places.I'm mostly relying on the tests in mattermost/mattermost#37758 to ensure that I haven't broken anything here, so fingers crossed we don't run into any issues with how plugins or anything else use these components.
Ticket Link
https://mattermost.atlassian.net/browse/MM-69835