Repository navigation
Conversation
… callback, not an Effect
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| Overlay · Storybook | 19.8 kB (−3 B) · 4.0 kB CSS (0) | 458 (0) | ✓ · ✓ · ✓ |
Package size: measured; main has no measurement of the merge base to compare yet.
API changes: none in this pull request
Committed API report behind this build: core 2 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: moving the radius copy from a layout Effect into the wrapper's ref callback is the right home for this work, but as written it changes behavior for one kind of caller.
Overlay.tsx:113:useMergedRefsrebuilds the merged ref whenever the caller'srefchanges identity, so with an inline callback ref (ref={el => …}) the radius copy runs again on every render, replacing a caller's laterstyle.borderRadiuswith the first child's radius and callinggetComputedStyleeach render, wheremaincopied once per mount.
Copying once per wrapper node (for example, skipping a wrapper that was already copied to) and adding a test that re-renders with an inline callback ref would make the "no behavior change" claim hold. A small non-blocking wording suggestion for the new comment is inline.
[Automated review]
| <div | ||
| {...rest} | ||
| ref={useMergedRefs(ref, overlay.containerRef)} | ||
| ref={useMergedRefs(ref, overlay.containerRef, mirrorFirstChildRadius)} |
There was a problem hiding this comment.
Blocking. useMergedRefs rebuilds this callback whenever the caller's ref changes identity. With an inline callback ref (ref={el => …}), React re-attaches it on every render, and mirrorFirstChildRadius reads getComputedStyle and writes the radius again. On main the copy ran once per mount. A caller's later style.borderRadius is replaced by the child's radius: 40px stays 40px on main and becomes 12px here. One way to copy once per wrapper node:
const copiedWrappers = new WeakSet<HTMLElement>();
function mirrorFirstChildRadius(container: HTMLElement | null) {
if (container == null || copiedWrappers.has(container)) {
return;
}
copiedWrappers.add(container);
const firstChild = container.firstElementChild;
if (firstChild == null) {
return;
}
const radius = getComputedStyle(firstChild).borderRadius;
if (radius && radius !== '0px') {
container.style.borderRadius = radius;
}
}A test that re-renders with an inline callback ref and a changed style.borderRadius would pin it.
| // The wrapper takes the radius of its first child, so the scrim clips to the | ||
| // media corners. Only the component does this. A hook consumer sets the | ||
| // radius on its own element. |
There was a problem hiding this comment.
Non-blocking: the first sentence restates mirrorFirstChildRadius. A version that keeps only the reason and the constraint:
// The scrim clips to the media's rounded corners.
// A useOverlay consumer sets the radius on its own element.
There was a problem hiding this comment.
Approval waits on one owner decision; nothing else blocks, and the earlier inline-ref blocker is fixed and pinned by the new re-render test.
Overlay.tsx:59-62: copying once per wrapper node also stopsmain's re-read of the first child when a hidden<Activity>or a re-suspended Suspense boundary shows the same Overlay again, so a radius changed while hidden or a late first child no longer reaches the wrapper while a radius set later on the wrapper now stays, and no current record says which behavior Overlay promises.
An engineering owner needs to confirm whether copy-once-per-wrapper-node is the intended contract on re-show; if it is, the change can land as is, ideally with the Changeset saying so instead of "Nothing a consumer imports or renders changes". A non-blocking wording suggestion for the new comment is inline.
[Automated review]
| if (container == null || wrappersWithRadius.has(container)) { | ||
| return; | ||
| } | ||
| wrappersWithRadius.add(container); |
There was a problem hiding this comment.
Owner decision needed. This guard also skips the copy when a hidden <Activity> or a re-suspended Suspense boundary shows the same Overlay again; React re-attaches the same wrapper there. On main the layout Effect re-ran at that point and re-read the first child: a child radius changed while hidden (12px to 48px) reached the wrapper, and a radius set later on the wrapper was replaced. Here the wrapper keeps its first copy (12px, and the caller's 48px stays). If once per wrapper node is the intended contract on re-show, this is fine as is.
| // The scrim clips to the media's rounded corners. Copy the radius once per | ||
| // wrapper node: a caller's inline callback ref re-runs the merged ref on each | ||
| // render, and a later radius from the caller must stay. |
There was a problem hiding this comment.
Non-blocking: the second sentence is 28 words with three topics. A version with one topic per sentence:
// The scrim clips to the media's rounded corners.
// Copy the radius once per wrapper node. An inline callback ref re-runs
// the merged ref on each render. A radius that the caller sets later stays.
// A useOverlay consumer sets the radius on its own element.
Maintainer impact
Why: no user action owns this work; it belongs to the moment the wrapper attaches.
Overlaycopies its first child's corner radius onto its wrapper so the clipped scrim follows the media's rounded corners. It did this in a mount-only layout Effect that read the wrapper from a ref. The read needs the wrapper element, so the wrapper's ref callback owns it (architecture:react-component-runtimeINV3 and INV6). The Effect also needed a lint suppression to write the style.Intended invariant
When the first child has a corner radius, the wrapper gets the same radius before the first paint. When the first child has none, the wrapper gets no inline radius. The caller's
refstill gets the wrapper.Change
What:
packages/core/src/Overlay/Overlay.tsx.useIsomorphicLayoutEffectinOverlay(about line 99) is removed, with its import and its lint suppression.mirrorFirstChildRadius(about line 57), does the same read and write. It joins the wrapper's merged ref after the caller's ref andoverlay.containerRef.WeakSet).useMergedRefsrebuilds the merged ref when the caller'srefchanges identity, so an inline callback ref re-runs it on every render. Like the mount-only Effect, the copy must not run again and replace a radius that the caller set later.React calls a ref callback during commit, after the children attach, which is the same phase as a layout Effect.
Evidence
refgets the same wrapper, and that a re-render with an inline callback ref keeps a radius set after mount (40px stays 40px; this test fails without the once-per-node guard and passes onmain). The existing Overlay tests pass without changes.Scope
No behavior change.
Risk
Low. Server rendering runs neither a ref callback nor a layout Effect, so it is unchanged.
useOverlayhook consumers are not affected; only the component copies the radius.Testing
vitest run --project ui --maxWorkers=1 packages/core/src/Overlay: 38 tests pass, 4 of them new.pnpm review:pre: 0 findings, merge into main clean.pnpm specs:for --diff origin/mainfollowed.tscfor the package and its docs), on main after test: main is green again: Tokenizer's sheet-row test passes presentation, and the recorded module resolutions follow core #9851: pass.