Skip to content

fix(portal): replay queued portal operations in order - #5048

Open
giaBaoJS wants to merge 1 commit into
callstack:mainfrom
giaBaoJS:fix/portal-host-queue-order
Open

fix(portal): replay queued portal operations in order#5048
giaBaoJS wants to merge 1 commit into
callstack:mainfrom
giaBaoJS:fix/portal-host-queue-order

Conversation

@giaBaoJS

Copy link
Copy Markdown

Summary

Portal.Host replays its pending-operation queue in the wrong order, so portals that mount in the same commit are stacked in reverse source order. A second bug in the same queue can drop a portal entirely.

Two lines in src/components/Portal/PortalHost.tsx, plus tests.

Why the queue is used at all

PortalHost renders <PortalManager ref={this.setManager} /> as a sibling after this.props.children. React runs the children's componentDidMount before the parent's, so every Portal that mounts in the first commit calls mount() while this.manager is still null and gets pushed onto this.queue. PortalHost.componentDidMount then drains it.

In other words the queue is not an edge case — it is the path every portal present on first render takes.

Bug 1: the queue is drained LIFO

while (queue.length && manager) {
  const action = queue.pop();   // last in, first out

Portal z-order is document order, so replaying the queue backwards stacks the portals backwards. With three sibling portals under one host, the rendered order today is third, second, first.

Bug 2: update() can clobber an unrelated queued mount

const index = this.queue.findIndex(
  (o) => o.type === 'mount' || (o.type === 'update' && o.key === key)
);

The first clause has no key check, so it matches the first queued mount of any portal. When a consumer mounts and then updates while the queue is still draining, the update overwrites some other portal's queued mount: that portal never appears, and the updated one is mounted twice under the same key.

The fix

-      const action = queue.pop();
+      const action = queue.shift();

-        (o) => o.type === 'mount' || (o.type === 'update' && o.key === key)
+        (o) => (o.type === 'mount' || o.type === 'update') && o.key === key

Both are queue-correctness bugs in the same function, so they are in one PR. They are independent, and each is covered by its own test — the tests are written so that reverting either line individually turns exactly one of them red.

Tests

Three tests in src/components/__tests__/Portal.test.tsx:

  • renders portals in source order when mounted in the same commit — three sibling Portals; asserts the rendered order is first, second, third. On main it is third, second, first.
  • stacks components mounted in the same commit in source order — the user-visible version, with a real Modal and Dialog in separate portals. On main the Dialog renders underneath the Modal.
  • keeps queued mounts of other portals when one of them is updated — covers bug 2. Deliberately order-independent (it asserts nothing is lost, not what order things land in) so it isolates the findIndex change rather than the drain-order change.

Verified by reverting each line on its own:

bug-1 order tests bug-2 test
both fixes pass pass
revert only shift()pop() fail pass
revert only the findIndex predicate pass fail

Full suite: 55 suites, 735 passed / 1 skipped, 169 snapshots — same 169 as before, no snapshot churn. yarn lint and yarn typecheck clean.

Behavioural impact — please read

This changes stacking order for apps that mount more than one portal in the same commit (for example a Modal plus a Snackbar plus a Dialog rendered together on first paint). Apps that were silently compensating for the reversal — reordering their JSX to get the layering they wanted — will see their layers flip.

I think source order is the correct semantics and worth the change:

  • It is what the component's own API implies: portals are ordered JSX siblings, and the later sibling paints on top, matching how every other React tree behaves.
  • The current behaviour is not even self-consistent. It only applies to portals present in the first commit; a portal mounted later goes straight through manager.mount() and is appended in the correct place. So today the same three portals stack one way on first render and the other way if they are mounted a tick later. That inconsistency is the part that is hardest to work around.

Still, it is a real behavioural change rather than a pure internal fix, so it may deserve a note in the changelog or a minor rather than a patch release. Happy to adjust.

Context

Related to #4647, which asks how to control the z-index of multiple portals. That issue is a question rather than a confirmed bug report and I do not want to overstate it — it is context for why the ordering matters, not a repro. The case for this PR is the code itself and the tests above.

`PortalHost` queues portal operations that arrive before its
`PortalManager` ref is attached, which is every portal that mounts in the
first commit. Two bugs in that queue:

- `componentDidMount` drained the queue with `pop()`, replaying the
  operations LIFO. Portals mounted in the same commit were therefore
  stacked in reverse source order.
- `update()` looked up the queued operation to replace with
  `o.type === 'mount' || (o.type === 'update' && o.key === key)`. The
  first clause ignores `key`, so an update for one portal overwrote the
  queued mount of an unrelated one, dropping it entirely.

Drain with `shift()` and key-match both branches of the lookup.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant