Repository navigation
docs(Overlay): say why the radius Effect stays, and pin when the radius is read - #9870
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 (0) · 4.0 kB CSS (0) | 458 (0) | ✓ · ✓ · ✓ |
Package size: @astryxdesign/core 6.30 MB packed (+235 B); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: theme-butter 1 row, theme-chocolate 1 row, theme-gothic 1 row, theme-matcha 1 row, theme-neutral 1 row, theme-stone 1 row, theme-y2k 1 row; 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.
Approval waits on one engineering-owner decision, and the Changeset then needs to match it.
- Owner decision: an
Overlayhidden and shown again in one synchronous run (twoflushSyncupdates, or a layout effect that shows it again in the hide commit) keeps its earlier corner radius wheremainreads the first child again, and no current record says which behavior Overlay promises, so an engineering owner (the author included) needs to say whether that divergence is accepted. - Changeset:
.changeset/overlay-radius-ref-callback.mdsays "Nothing a consumer imports or renders changes", which does not hold for that case, so after the decision either the Changeset (and the "No behavior change" line) names the case or the attach guard covers it.
Everything else checks out: the five new tests also pass on main, and the hovered card overlay renders identically before and after. Two small comment wording suggestions are inline and non-blocking. CI runs once the pull request leaves draft.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: `Overlay` copies its first child's corner radius onto its wrapper from the wrapper's ref callback, not from a layout Effect. The radius still lands before the first paint. Nothing a consumer imports or renders changes. |
There was a problem hiding this comment.
"Nothing a consumer imports or renders changes" does not hold when an Overlay is hidden and shown again in one synchronous run: it keeps its earlier radius, where main reads the child again (the Risk section says so too). Once an engineering owner decides on that case, please make this sentence match, for example: "An Overlay hidden and shown again in one synchronous run keeps its earlier radius."
| // Read the radius each time React attaches a wrapper, also when a hidden | ||
| // Activity shows it again: the child can change while the wrapper is hidden. |
There was a problem hiding this comment.
Non-blocking: this is one 26-word instruction with two topics. A shorter version that keeps the reason:
// Read the radius on each attach. The child can change while the wrapper is hidden.| // wrapper in one commit. That is not a new attach. The detach mark lasts until | ||
| // the commit ends, so the attach in the same commit does not read again. |
There was a problem hiding this comment.
Non-blocking: the mark lasts until the next microtask, not until the commit ends, and that difference is the synchronous hide-and-show case. Suggested:
// The detach mark lasts until the next microtask. An attach before then does not read again.
cixzhang
changed the title
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet because the pull request is a draft, and marking it ready for review starts it.
The new comment explains why the radius Effect stays, and the code under it is unchanged from main. Two optional suggestions:
- The Activity test's
await act(async () => Promise.resolve())(Overlay.test.tsx:317) puts a microtask between the hide and the show, so the microtask-guarded ref callback this pull request rules out still passes all five new tests; dropping that line, and adding a re-suspended Suspense case, makes the tests catch it. - In
Overlay.tsx, "This is a layout read of the mounted DOM, and no user interaction causes it." reads better as two sentences.
[Automated review]
| expect(root.style.borderRadius).toBe('12px'); | ||
|
|
||
| rerender(overlay('hidden')); | ||
| await act(async () => Promise.resolve()); |
There was a problem hiding this comment.
Non-blocking: this microtask separates the hide from the show, so the microtask-guarded ref callback the description rules out passes this test, and the whole suite. Without this line the Effect still passes, and that ref callback fails (12px instead of 48px). A re-suspended Suspense case would pin the other re-show the description lists.
| // the media's rounded corners. This is a layout read of the mounted DOM, and | ||
| // no user interaction causes it. It runs again when a hidden Activity shows |
There was a problem hiding this comment.
Non-blocking: one topic per sentence reads better here:
// This is a layout read of the mounted DOM. No user interaction causes it.
Maintainer impact
Why: the Effect ledger marked
Overlay's radius Effect as a move to the wrapper's ref callback. Two tries showed that a ref callback cannot keep the Effect's timing, so the Effect stays, and its source comment now says why (architecture:react-component-runtimeINV3). No user interaction causes this work.Overlaycopies its first child's corner radius onto its wrapper, so the clipped scrim follows the media's rounded corners. The required timing is: read on mount, read again when a hidden<Activity>or a re-suspended Suspense boundary shows the wrapper (the child can change while it is hidden), and do not read on a plain re-render.ref, becauseuseMergedRefsrebuilds the merged ref. That replaced a radius set after mount (refactor(Overlay): take the first child's radius in the wrapper's ref callback, not an Effect #9861, first version).<Activity>shows the wrapper again (refactor(Overlay): take the first child's radius in the wrapper's ref callback, not an Effect #9861).Intended invariant
The wrapper takes the first child's radius before the first paint, takes it again when a hidden
<Activity>shows it, and keeps a radius set after mount across re-renders, also with an inline callbackref.Change
What:
packages/core/src/Overlay/Overlay.tsx, the comment above theuseIsomorphicLayoutEffectinOverlay(about line 97). The code is unchanged.packages/core/src/Overlay/Overlay.test.tsx: five new tests pin the timing above, so a later move to a ref callback has the cases to pass.Evidence
main. The 12px child radius reaches the wrapper; a child with no radius gives none; a callbackrefgets the wrapper; an inline callback ref re-render keeps a later 40px radius; a hidden<Activity>shown again gives the wrapper the child's new 48px radius.Scope
No behavior change.
Risk
None for consumers: comment and tests only.
Testing
vitest run --project ui --maxWorkers=1 packages/core/src/Overlay: 39 tests pass, 5 of them new.pnpm review:pre: 0 findings. Core typecheck: pass.