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

@giaBaoJS giaBaoJS commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

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.

One line 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.

The bug

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.

The fix

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

Tests

Two tests in src/components/__tests__/Portal.test.tsx, both public behaviour only: real Portal elements under a Portal.Host, assertions on what is rendered.

  • renders portals in source order when mounted in the same commit asserts the three rendered portals read first, second, third. On main they read third, second, first.
  • stacks components mounted in the same commit in source order is the user-visible version, with a real Modal and Dialog. On main the Dialog renders underneath the Modal.

Reverting the one-line change turns both red, with the reversed text visible in the failure output.

Full suite: 55 suites, 679 passed / 1 skipped, 168 snapshots, no snapshot churn. yarn lint, yarn typecheck and prettier --check are 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, by 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.

Changed since review

@satya164 flagged the third test as testing implementation details. That test drove the queue through PortalContext directly, and it was the only cover for a second change that used to be in this PR: the update() lookup matched the first queued mount of any key instead of the given key.

Before rewriting it I checked whether that second bug is reachable without touching the context. I instrumented the queued update() branch and ran the whole suite: nothing except the removed test reaches it. PortalConsumer only calls update() from componentDidUpdate, and React flushes a layout-phase setState after all of the commit's layout effects, by which point the manager ref is attached, so a plain Portal can never call update() while the queue is still live.

Rather than ship a change I cannot cover with a public-behaviour test, I dropped it. This PR is now the one-line ordering fix only, which matches its title. Happy to raise the other one separately if you want it as hardening.

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.

@giaBaoJS

Copy link
Copy Markdown
Contributor Author

Thanks @ziarno.

Same situation as #5046: ci.yml has never been dispatched on this branch, so the required checks are still empty and the PR cannot merge on the approval alone. I wrote up what I found over there rather than repeating it here.

Nothing needed from me that I can see. Branch is clean against main and I am happy to rebase if that helps get a run going.

Comment on lines +69 to +70
// The last portal in source order is painted on top, so `dialog` has to come
// after `modal` in the rendered tree.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// The last portal in source order is painted on top, so `dialog` has to come
// after `modal` in the rendered tree.

Comment on lines +77 to +123

it('keeps queued mounts of other portals when one of them is updated', async () => {
// Mirrors `PortalConsumer`: mounts from `componentDidMount`, then updates its
// own key. Both calls land before `PortalHost` attaches its manager, so they
// go through the queue.
class QueuedConsumer extends React.Component<{
manager: PortalMethods;
label: string;
update?: boolean;
}> {
componentDidMount() {
const key = this.props.manager.mount(
<Text testID="queued">{this.props.label}</Text>
);

if (this.props.update) {
this.props.manager.update(
key,
<Text testID="queued">{`${this.props.label} (updated)`}</Text>
);
}
}

render() {
return null;
}
}

await render(
<PortalHost>
<PortalContext.Consumer>
{(manager) => (
<>
<QueuedConsumer manager={manager} label="first" />
<QueuedConsumer manager={manager} label="second" update />
</>
)}
</PortalContext.Consumer>
</PortalHost>
);

// Deliberately order-independent: this asserts that no portal is lost, which
// is a separate concern from the order the queue is replayed in.
expect(screen.getAllByTestId('queued')).toHaveLength(2);
expect(screen.getByText('first')).toBeTruthy();
expect(screen.getByText('second (updated)')).toBeTruthy();
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is testing implementation details. please update it to use public behavior by checking what's rendered and performing operations with a normal protal without consuming the portal context directly

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewritten: the tests now render plain <Portal> elements inside a Portal.Host and assert on the rendered output with findAllByTestId + toHaveTextContent, so nothing touches the portal context. Order is observable from the outside because PortalManager renders its portals in array order, so a LIFO replay puts third, second, first into the tree and the assertion fails on visible text; the Modal/Dialog test is the same thing with real components.

I also dropped the second fix and the test you flagged. I instrumented the queued update() branch and ran the full suite: nothing but that test reaches it, since PortalConsumer only calls update() from componentDidUpdate and React flushes a layout-phase setState after the manager ref is already attached. I would rather not ship a change I cannot cover with public behaviour, so this PR is now just the one-line ordering fix.

`PortalHost` queues portal operations that arrive before its
`PortalManager` ref is attached, which is every portal that mounts in the
first commit. `componentDidMount` drained that queue with `pop()`,
replaying the operations LIFO, so portals mounted in the same commit were
stacked in reverse source order.

Drain with `shift()` instead.
@giaBaoJS
giaBaoJS force-pushed the fix/portal-host-queue-order branch from 46879a8 to c360ef7 Compare September 3, 2026 11:55
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.

3 participants