Repository navigation
fix(builder): contain portalled overlays inside the shell's inert boundary - #892
Conversation
…ndary An overlay leaves the DOM position it was opened from, so the shell's `hidden` and `inert` could never reach it: an open Select in the inspector stayed visible and clickable on top of the notice saying the editor was unavailable. Fixed by redirecting where those overlays portal to rather than by teaching each control to close itself. The shell publishes a host inside the subtree that carries `hidden`/`inert`, and `usePortalContainer` already resolves through context, which reaches through portals. Every portalling component inherits the boundary, including ones added later, which a rule written on seven call sites would not cover. Where the host sits is the design. Inside a region clips it, because the panel, the canvas and the inspector are each `overflow-auto`. Inside the shell root clips it too, since that root is `overflow-hidden`. It is a sibling of the shell root: within the element carrying the attributes, and outside every box that clips. Taken out of flow so it contributes no layout to the wrapper it sits beside, which is `display: contents` while visible and would otherwise gain a phantom child box. Clipping only becomes possible once an ancestor is a containing block for fixed positioning, and none exists today, so a test asserting the overlay renders cannot distinguish a correct placement from a clipped one. The unit guard asserts the condition instead: no ancestor of the host carries a transform, filter, contain or will-change. The day canvas zoom adds one, that fails rather than overlays silently mispositioning. The harness gains its one interactive slot, a Select at the bottom of the scrolling inspector, because where an overlay lands is not decidable from markup and jsdom computes no styles.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex please review this PR |
|
@nextly-bot review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 2 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
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 |
1 participant
Closes the finding Codex raised on #884 and I deferred there. It was live on
main, and it is the third round of the same shape, so it is fixed as a boundary rather than as a third patch.The defect
The shell sets
hiddenandinerton its own subtree when it has too little width. Both are attributes on an element and reach descendants only. A portalled overlay is not a descendant of anything inside the shell —SelectContent,DropdownMenu,ContextMenuand every other portalling component resolve their container fromPortalProvider, which the admin points at one root near the document body. So an open dropdown in the inspector stayed fully visible and clickable on top of the notice saying the editor was unavailable.Reproducible on
maintoday by narrowing the window with a Select open.Why not the suggested fix
The review asked for the controls to consume
useShellIsActive. That is 7<Selectsites across 3 files, and the eighth control is written by someone who never read the rule.AGENTS.mdprefers a boundary the system cannot cross to a check that looks for crossings, so the container is redirected instead and every present and future overlay inherits it. React context reaches through portals, which is what makes this possible without touching a single call site.packages/uineeded no change at all:usePortalContainerreturnscontainer ?? undefined, so a nested provider simply shadows. Verified that nothing keys on there being one portal root —nextly-admin-portal-roothas exactly one occurrence repo-wide, its own definition.Where the host sits is the whole design
Three placements, two of them wrong for reasons that are silent:
overflow-auto, so an overlay opened near an edge is cut off. Worse than the bug being fixed, because it is visual rather than interactive.overflow-hidden.hidden/inert, outside every box that clips.Taken out of flow, because the wrapper it sits in is
display: contentswhile visible and a real child box would become a phantom flex or grid item in the shell's layout parent.The guard, and what it is standing in for
Clipping only becomes possible when an ancestor is a containing block for fixed positioning. There is none in the shell today, which means a test that opens an overlay and asserts it rendered cannot distinguish the correct placement from a clipped one — measured, not assumed: with the host moved inside the inspector, the browser test still passes.
So the placement is guarded by asserting the CONDITION instead: no ancestor of the host carries a
transform,filter,contain,will-changeor the rest. The same properties create a stacking context, which would additionally make every z-index inside the host local and unable to compete with admin-level overlays. A page builder gets canvas zoom eventually; that guard fails on the day it lands rather than leaving overlays quietly mispositioned.Testing
Unit,
packages/builder/src/builder-shell.test.tsx(4 new), each stub-verified:hidden/inert— moving it outside fails this and nothing elseBrowser,
e2e/tests/shell/shell.spec.ts(4 new). The harness gains its one interactive slot — a Select at the BOTTOM of the scrolling inspector — because where an overlay lands is not decidable from markup and jsdom computes no styles:closest()resolves to the shell's host, so it landed there rather than at the bodyOne thing found while writing those: a Radix
Select, while open,aria-hiddens the rest of the page, so a test cannot read the inspector's box at the same time as the dropdown's. My first version did and timed out.Not covered, stated rather than implied
Containment hides a modal overlay but leaves it MOUNTED, so a focus-trapping overlay could in principle run its trap inside a
display: nonesubtree. I have not driven that case, and it is the one place where being told — the existinguseShellIsActive, whichMediaControlalready consumes — is stronger than being contained. The two are complementary rather than alternatives, and the remaining work is to decide which overlays need telling as well.Verification
packages/builder: 374/374, typecheck, lint cleanpackages/plugin-page-builder: 828/828, both tsconfig programsapps/playground: typecheck and lint cleane2e/tests/shell/shell.spec.ts: 24/24 in Chromiumnpx sherif: 0 errorsscripts/release/check-changesets.mjs