[toast] Replace custom data wholesale and accept a function to derive it - #5611
Conversation
commit: |
Bundle size
PerformanceTotal duration: 1,061.92 ms -117.36 ms(-10.0%) | Renders: 76 (+0) | Paint: 1,727.69 ms -186.11 ms(-9.7%) No significant changes — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
`updateToastInternal` object-spread `data` on every update that carried one.
`Data extends object = any`, so `data` may legitimately hold an array, a `Map`,
or a class instance, and spreading those silently converts them to plain
objects — index-wise for arrays, so a shorter update leaves stale trailing
entries, and to `{}` for a `Map`, losing everything.
Merge only when the stored value and the update are both plain objects, and
replace wholesale otherwise, which is what 1.7.0 did. The duplicate-id `add`
path also stops merging: `ToastManagerAddOptions.data` is a complete `Data`,
not a patch, so keys the caller omits must not survive.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c64c27a to
57d493e
Compare
michaldudak
added
type: regression
`updateToast` inspected and merged `data` before `updateToastInternal` rejected updates for missing or ending toasts, so a promise settling after its toast was dismissed still read the caller's getters. Check the toast first. `mergeData` used `Object.assign`, which writes through inherited setters: an own `__proto__` key (as `JSON.parse` produces) swapped the merged object's prototype and was dropped, and the result then failed `isPlainObject` so later patches replaced instead of merging. Spread the values so every key becomes an own data property, then restore the previous prototype. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`isPlainObject` treated any object whose prototype was one hop from `null` as a plain record, which also matched instances of a class whose prototype chain was cut off right after the class, so those merged instead of being replaced as documented. Check that the prototype is a realm's `Object.prototype` instead: either this realm's, or one whose own `constructor` is that realm's native `Object`. The cross-realm tests now use a real iframe realm rather than a stand-in `Object.create(null)` prototype, which the tighter check rightly rejects. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`ToastManagerUpdateOptions.data` was typed `Partial<Data>` while the stored `ToastObject.data` stayed a complete `Data`, so an incomplete value replaced a class instance or filled an empty toast and consumers reading `toast.data` crashed on the missing keys. `data` is a complete `Data` again and always replaces, on `update`, `promise`, and duplicate-id `add` alike. Shallow merging moves to a new `dataPatch` option, which is only applied when there is a plain object to patch (the toast's data, or the `data` given alongside it); otherwise it is dropped with a development warning. That keeps the stored type truthful: data is either a complete value or a complete value with some keys overwritten. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
michaldudak
changed the title
…ract Recognizing another realm's `Object.prototype` and restoring the previous prototype after a merge cost about 115 gzipped bytes for a case no consumer reaches, and the fallback without it is the same development warning every other non-plain value gets. `isPlainObject` is now a same-realm check and the merge is an inline spread; the two iframe tests go with it. Rejecting a `null` prototype is now a necessity rather than a policy, since a spread would swap it for `Object.prototype`. The promise `loading` options omit `dataPatch` at the type level, because the loading toast is always new; the runtime strip stays for JavaScript callers. Both spec files assert the rejection. The `dataPatch` JSDoc and the warning now say that both the stored data and the patch must be plain objects and list null-prototype objects among the rejected values, and `ToastManagerUpdateOptions.data` carries its own JSDoc stating that it replaces the stored value. The `update` section of the docs page gained a partial-update example, and `types.md` is regenerated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Wrapping the `warn` call in a `process.env.NODE_ENV` guard lets a consumer's bundler strip the message, and since `@base-ui/utils` declares no side effects, the now-unused `warn` import and `createLogOnce` go with it. On the store module alone that is about 180 gzipped bytes, plus the logger's shell for apps that use no other component importing it. The warning still fires in development. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b193a8412
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Folding `resolveDataPatch` into `updateToast` removes the helper, its return object, and the loading-path strip in `promiseToast`, which the `loading` type already makes unreachable from TypeScript. A `dataPatch` passed there from JavaScript now stays on the toast as an inert key instead of being dropped with a warning, so the test half covering that path is gone. `isPlainObject` only needs to guard `null` and `undefined`, since primitives resolve to their own prototype, and the fresh rest object makes `in` equivalent to `Object.hasOwn` for the base lookup. The toast entry now costs about 78 gzipped bytes over the merge base, down from 361 as first submitted. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
atomiks
changed the title
| * Custom data for the toast. | ||
| * Replaces the stored value. Pass a function to derive the next value from the current one. | ||
| */ | ||
| data?: Partial<Data> | undefined; | ||
| data?: Data | ((prevData: Data | undefined) => Data) | undefined; | ||
| } |
There was a problem hiding this comment.
dataPatch seemed odd; refactored to a function updater here
michaldudak
changed the title
The updater receives `Data | undefined`, so spreading `prevData` in the docs
example made every other property optional and failed to type-check for any
`Data` with another required key. Letting the function return `undefined`,
mirroring `data: undefined`, allows the guarded form
`(prevData) => prevData && { ...prevData, progress: 100 }`, which compiles and
reads as intended. The JSDoc now states both `undefined` cases.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`Data extends object` admits function types, and `resolveData` calls any function it is given, so the `data` JSDoc now says a function is always treated as an updater and never stored as the value. Both type specs also assert that `add` rejects an updater, since its `data` is a plain `Data`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63dee7ca2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2484792a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The JSDoc promises the function form of `data` may return `undefined` to clear the stored value, and the docs example relies on it. Pin it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Resolve the function form of `data` in `updateToast`, so `updateToastInternal` only ever receives a plain value and goes back to a plain spread. This fixes two review findings at once: - A function-valued `data` given to `add` under an existing id is stored as the value again, as its type promises, instead of being run as an updater. - An updater that calls back into the store (`add`, `close`, `update`) no longer has its work clobbered: the internal update re-reads the current toast and array after the updater ran and repeats the missing/ending guard. The updater is still never called for a missing or ending toast. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 821dce5804
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // `data` is a plain value now, and the updater may have called back into the | ||
| // store, so the internal update reads the current state again. | ||
| this.updateToastInternal(id, resolved as ToastInternalUpdateOptions<Data>, false, true); |
There was a problem hiding this comment.
When a data updater calls close(id) and then add({ id, data: replacement }), addToast removes the ending toast and creates a new one, but this call subsequently finds the replacement by the same ID and applies the old toast's resolved data to it. The fresh evidence beyond the earlier nested-mutation report is that the new post-callback lookup is keyed only by id, so it cannot distinguish the original toast from a replacement and can overwrite the replacement's data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduced: after close(id) followed by add({ id, data }) inside the updater, the updater's return value lands on the recreated toast and bumps its updateKey. Leaving this as is.
The return value is the caller's explicit next data for the id being updated, so applying it to whichever toast carries that id when the update completes is last-write-wins, and an updater that recreates the toast can return the replacement's data if that is what it wants.
Fixing it properly is not cheap. Comparing object identity before and after the callback cannot separate "recreated" from "updated in place", because a nested update on the same toast also produces a new object and that case must still apply the outer result. Telling them apart needs a per-toast instance token stamped in addToast and checked in updateToast, which costs bytes on every toast for an updater that destroys and recreates the toast it is deriving data for. Bailing out whenever the toast changed during the callback would instead drop the outer data whenever the updater sets any other field on the same toast, which is the likelier case.
`Data extends object` includes function types, so `update(id, { data: fn })`
type-checked as a value while the store called it as an updater and stored
the result. Exclude callable `Data` from the value arm of
`ToastManagerUpdateOptions.data`: storing a function now means returning it
from an updater, the same convention as React state setters. `add` still
takes the function as a value.
Type-only change; the minified toast bundle is byte-identical.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 participants
The bug
ToastStore.updateToastInternalobject-spread the toast'sdataon every update that carried one:The public generic is
Data extends object = any, sodatalegitimately holds arrays,Map/Set, and class instances — not just plain records. Spreading those silently converts them to plain objects, and for arrays it merges index-wise, so a shorter update leaves stale trailing elements behind.datamaster['x', 'y']['z']{"0":"z","1":"y"}— not an array, stale'y'survives['z']new Model('a')new Model('b')greet()gonenew Map([['k',1]])new Map([['j',2]]){}— every entry lostMap, intactThis is a regression against 1.7.0, where
...updatesreplaceddatawholesale. The merge came in with #5464 ("[toast] Support partial data updates"), which is unreleased.The same commit also typed
ToastManagerUpdateOptions.dataasPartial<Data>whileToastObject.datastayed a completeData. Any partial value — a fragment of a class instance, or a patch for a toast that has no data yet — type-checks, is stored as the whole value, andtoast.data.greet()then throws even though the types say it is there.A second instance of the merge regression sits on the
addpath: re-adding a toast under an existing id kept keys the caller had deliberately dropped, sinceToastManagerAddOptions.datais a completeData, not a patch.The fix
datareplaces; a function derives.ToastManagerUpdateOptions.datais a completeDataagain and always replaces — onupdate, onpromisesettlement, and on duplicate-idaddalike, matching 1.7.0. A partial update is written as a function that receives the current value and returns the next one:The function receives
undefinedwhen the toast has no data yet and may returnundefinedto clear the value, mirroringdata: undefined. Because the caller owns the merge, everyDatashape works — arrays,Maps, class instances — with no plain-object detection, no development warning, and no logger in the bundle.Ignored updates never call the function.
updateToastresolves it only after checking that the toast exists and is not ending, so a promise settling after its toast was dismissed never runs it. The internal update then re-reads the store, so an updater that calls back into the manager (add,close,update) does not have its work overwritten.promiseresolves aloadingfunction againstundefined, since that toast is new.An earlier revision of this PR added a separate
dataPatch: Partial<Data>option that shallow merged into plain-object data only; the function form ofdatareplaces it.JSDoc, the type specs for
useToastManagerandcreateToastManager, theupdatesection of the docs page, and the regeneratedtypes.mdall reflect this.Tests
packages/react/src/toast/store.test.ts, passing in jsdom and Chromium:datareplaces wholesale,undefinedclears, and omitting it leaves the value aloneupdateKeyundefinedwhen the toast has no datadataloadingandsuccessfunctions on a promise toast resolve against the loading dataType specs assert that an incomplete
datais a type error, that the function receivesData | undefined, that an incomplete return is a type error, and that a callableDatais rejected as a direct value onupdateandpromisewhile the wrappeddata: () => fnform is accepted. Full toast suite in jsdom: 306 passed / 38 skipped.Bundle size
@base-ui/react/toastentry, esbuild in production mode with React external: 20666 gzipped bytes against 20635 at the merge base, +31. The PR as first submitted was +361.Breaking change?
Partial<Data>ondatacame in with #5464, which has not shipped, so reverting it is not a released change. Against 1.7.0, the only behavior change is that a function passed asdatatoupdate, or to theloading/success/erroroptions ofpromise, is now called as an updater instead of stored;addstill stores it as a value. This only affects aDatathat is itself a function type. For that shape the types now reject a callable value on those paths, so the change surfaces as a compile error rather than at runtime: return the function from an updater (data: () => fn) to store it.🤖 Generated with Claude Code