fix(toast): render the react root through the factory so asChild works - #4072
aaron-easygo wants to merge 1 commit into
Conversation
|
@aaron-easygo is attempting to deploy a commit to the Chakra UI Team on Vercel. A member of the Team first needs to authorize it. |
| </> | ||
| ) | ||
|
|
||
| // The ghosts belong to the root, so asChild has to nest them inside the child it renders as. |
There was a problem hiding this comment.
Comment violates repository guide
This explanatory inline comment violates the repository directive to avoid comments and follow the existing minimal-commenting pattern. This repository requirement must be satisfied before merging; remove the comment and keep the implementation self-explanatory.
| // The ghosts belong to the root, so asChild has to nest them inside the child it renders as. |
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react/src/components/toast/toast-root.tsx
Line: 24
Comment:
**Comment violates repository guide**
This explanatory inline comment violates the repository directive to avoid comments and follow the existing minimal-commenting pattern. This repository requirement must be satisfied before merging; remove the comment and keep the implementation self-explanatory.
```suggestion
```
**Context Used:** CLAUDE.md ([source](https://github.com/chakra-ui/ark/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Toast.Root declared PolymorphicProps but rendered a plain div, so asChild was never read: the child stayed nested inside an extra root element and the prop was spread onto the DOM, which React warns about. It is the only React part that claims asChild without going through the factory. The machine's ghost elements belong to the root, so they are nested inside the child the root renders as rather than left as siblings of it. With the root going through the factory, toast/toast-root now reads on all four adapters (div everywhere), so it comes off knownUnreadableRoots in check-nodes — the list is shrink-only, and leaving it there fails the check.
83888da to
94448aa
Compare
|
Rebased onto main (post-#4084) and dropped Worth noting for triage: v6 already renders |
Toast.Rooton React declaresPolymorphicPropsbut renders a plain<div>rather thanark.div, soasChildis never read. Two consequences:asChildis spread onto the DOM, which React warns about:React does not recognize the asChild prop on a DOM element.Before, for
<Toast.Root asChild><section data-testid="custom-root">…</section></Toast.Root>:After:
It is the only React part that claims
asChildwithout going through the factory — I found it while widening thecheck:nodesscript in #4071, which flags parts whose root is not anarknode.The ghosts
The reason this part never moved to the factory is presumably its two ghost elements. They belong to the root — their job is to keep the pointer inside the group between toasts — so with
asChildthey have to end up inside the child, not as siblings of it. Handing the factory a fragment of three children would not work: it would clone the fragment and React rejects props onReact.Fragment. So the root clones the child with the ghosts around the child's own children, and the factory still does the prop merge and ref composition.If
asChildis set and the child is not a valid element, it passes through untouched and the factory returnsnull, same as every other part.Tests
should render the asChild element as the root, keeping the ghosts inside it— asserts thesectioncarriesdata-part="root", that there is exactly one[data-part="root"]in the document, that noaschildattribute is emitted, and that the ghosts are the child's first and last elements. It fails onmainand passes here.bun run --cwd packages/react test,typecheck, biome and prettier are clean.Not in this PR
The other three adapters cannot honour
asChildon this part either, in three different ways:Dynamicclones the first non-comment child, which here is the ghost-beforediv, so the root props land on the ghost and all three children render as siblings.asChildis a render prop / snippet that replaces the factory's own children, so both ghosts disappear.Those need a decision rather than a patch (the render-prop shape gives the part no way to inject children into the consumer's element), so I have left them alone. Happy to open an issue for it, or to follow this up if you have a direction in mind.
If #4071 lands first, this PR also needs
toast/toast-rootdeleted fromknownUnreadableRootsinscripts/src/check-nodes.ts— that list is shrink-only, so the check fails until the line goes. Say the word and I will rebase whichever lands second.The behavioral fix appears sound, but the explicit repository commenting requirement must be satisfied before merging.
Fix with agent prompt
Summary
This PR fixes React
Toast.Rootpolymorphism by rendering throughark.divand injecting the toast positioning ghosts into the selected child before factory prop and ref composition.asChildelement the actual toast root.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Toast.Root props] --> B{asChild?} B -- No --> C[Wrap ordinary children with before/after ghosts] B -- Yes --> D[Clone valid child with ghosts around its children] C --> E[ark.div factory] D --> E E --> F[Merge toast and child props] E --> G[Compose forwarded and child refs] F --> H[Rendered toast root] G --> HReviews (1) · Last reviewed commit: "fix(toast): render the react root throug..."