feat(host): wrap each inline sheet, so an app can put an error boundary around one sheet

[VadymBezpalko]

Closes #51.

What. BottomSheetHost takes SheetWrapper?: ComponentType<SheetWrapperProps>; QueueItem renders it around each inline sheet's content, inside the sheet's context, with the ref the coordinator drives (SheetWrapperProps = { id, sheetRef, children }, sheetRef: SheetRef | undefined). useAdapterRef catches up on mount: an adapter mounting under an already-open sheet expands itself, and one mounting under closing ends the sheet in the store, so a fallback adapter rendered in place of a throwing sheet comes up open, and a throw mid-close cannot leave the sheet stuck.

Why. A throw in an inline sheet's body could only be caught above the host, which unmounts every sheet in the group and leaves nothing on screen. With the wrapper an app catches per sheet and shows a fallback sheet with Close / Retry; the host and the other sheets never notice.

Scope. Inline sheets only — portal and persistent content renders at its declaration site, so the wrapper is applied in the non-portal branch. Apps that do not pass SheetWrapper see two changes, both in states that were dead sheets before: an adapter mounted under open that nobody drives now opens, and one mounted under closing (also reachable today via BottomSheetPortal, which has no unmount cleanup) ends the sheet instead of leaving a closing record that closeAll skips, requestClose refuses and no back handler covers. hidden is deliberately not handled: switch-parked and persistent sheets live there and are restored by the store.

Review round. Six commits on top of the original five, one per thread, in dependency order:

• open() reuses the ref already registered for an id, so one id maps to one ref object and QueueItem's cached render-time read cannot go stale; this also stops open(x, { id }) from clobbering a mounted persistent sheet's ref.
• open() registers the ref before the store write and reclaims only its own entry on rejection; the QueueItem comment states the real invariant.
• The wrapper renders unconditionally in the inline branch; sheetRef (and useAdapterRef's parameter) may be undefined.
• useAdapterRef catch-up handles closing with finishClosing(id).
• __DEV__ warning when the SheetWrapper prop value changes between renders (which remounts every inline sheet); the requirement is a stable value, not only a module-scope component.
• The example fallback no longer paints its own scrim; components.md notes the animatedIndex rewind a zero-seeded fallback adapter causes.

Reviewer notes.

• QueueItem reads getSheetRef(id) during render, cached by the compiler on [id, usePortal]. open() registers the ref and then writes the store; React only schedules the render that write causes, so the first render sees the entry. The read is guarded on usePortal because a persistent id can be re-registered under a still-mounted item.
• SheetWrapper must be a stable prop value (a module-scope component, not an inline arrow or a branch on a flag) — QueueItem is memo and uses it as an element type, so a new value remounts every inline sheet. Documented on the prop and warned in dev.
• The example's SheetErrorBoundary renders CustomModalAdapter bound to sheetRef, with Close (useBottomSheetContext().close()) and Retry (setState). Not run on a device here — the Retry path (the real sheet remounts and is re-expanded by the same catch-up) is the part worth a smoke test.
• While writing the tests I hit what looks like a React Compiler bug: a useImperativeHandle factory declared inside another function is outlined to module scope and loses its closure, throwing ReferenceError at commit time. The test helpers live at module scope for that reason. Library components are unaffected.

Tests. src/__tests__/host.test.tsx, adapterRef.test.tsx, hooks.test.tsx cover: no wrapper → content unchanged; wrapper receives id / sheetRef / sheet context; portal sheets not wrapped; wrapper renders with sheetRef: undefined when nothing is registered; one sheet replaced by a boundary while the other stays mounted; a fallback adapter replacing a sheet that throws after opening is driven; a throw mid-close ends the sheet; hidden catch-up stays a no-op and a switch-parked sheet is still restored; same-tick re-open and persistent-id re-open keep one ref per id; a rejected re-open keeps the live sheet's ref; the dev warning fires once for a changing wrapper and never for a stable one. Each new test was checked to fail without its change.

yarn typecheck, yarn lint, yarn test (9 suites / 140 tests, from 7 / 122 on main), yarn prepare green.

[arekkubaczkowski]

Reviewed the whole diff and ran the branch locally — yarn test is green (9 suites / 131 tests, matches the description). The design is sound and scoping the catch-up to 'open' is the right call. Six comments inline, roughly in order of severity.

Two things I verified rather than assumed, since both bear on the inline comments:

• I compiled QueueItem.tsx through babel-plugin-react-compiler to check what happens to the render-time registry read. It is cached on [id, usePortal] (one $[3]/$[4]/$[5] slot), so it is read exactly once per mounted item and never re-run — which makes the two failure modes below permanent rather than transient.
• I wrote a throwaway test that opens through the public useBottomSheetManager().open() and asserted the wrapper receives a ref. It does, so the batching assumption holds today.

Verdict: happy to approve once (1) and (2) are addressed, or once (1) is consciously accepted as a documented limitation — but then the dead back handler is worth writing down.

[arekkubaczkowski]

A throw during closing leaves a permanent zombie sheet, and kills the Android back button for the whole group.

This is the open question from the description, but the blast radius is larger than the description suggests, and I do not think the give-up path saves it.

The sequence: the sheet is open, the user closes it, and driveSheetRef(id, 'closing', ref.close) succeeds on its first attempt against the old adapter (its ref.current is set) and returns. Its 10-frame give-up path — the thing that would otherwise call finishClosing — is therefore already gone. The content then throws mid-animation, the boundary swaps in a fallback adapter, and nothing ever calls close() on it: the coordinator subscription fires on status changes only, and the status is already 'closing'. handleClosed never runs, so finishClosing never runs.

What that costs, all of it checked against main:

• closeAllAnimated skips it (bottomSheetCoordinator.ts:249 — status === 'closing' → continue) and requestClose returns not-closable for it (:110), so closeAll() can never clear it. Only destroyAll() can.
• useIsTopmostAndOpen requires status === 'open' and being last on the group stack (store/hooks.ts:65-71). The zombie is on top but is not open, so it registers no back handler — and the sheet below it is no longer topmost, so it registers none either. On Android the hardware back button stops closing anything in that group.

The window is real rather than theoretical: driveSheetRef only covers roughly the first 10 frames, and a close animation runs ~300 ms (~18 frames), so a throw in the back half of the animation lands here.

finishClosing(id) in the catch-up for closing / hidden, exactly as driveSheetRef's give-up path does, closes the gap.

Small correction to the description while I am here: this does not "wedge every later open in the group on the group-busy guard" — that guard is on 'opening' only (store.ts:88). The zombie and the back handler are the real costs.

[VadymBezpalko]

Confirmed, and the window is the whole animation rather than the back half: once ref.current is set on the first attempt, driveSheetRef returns and the give-up path never arms. It is also reachable on 2.1.1 without SheetWrapper — BottomSheetPortal has no unmount cleanup (unlike BottomSheetPersistent), so {visible && <BottomSheetPortal/>} cleared mid-close leaves the same 'closing' record, and that id returns already-active for the rest of the session; worth its own issue, I have not opened one.

Added finishClosing(id) to the catch-up for 'closing' only. Not 'hidden': a switch-parked sheet sits at 'hidden' on the group stack and is restored by detachFromGroup when the sheet above closes, so finishClosing there would delete it, and mount() parks every persistent sheet at 'hidden' with its adapter rendered, which would fire a stack-rewriting write on every persistent mount. 'hidden' is terminal; nothing is stuck there.

The group-busy sentence you corrected is the pre-existing one in CLAUDE.md, not the PR body — fixed in the same block. components.md now states that the fallback must render an adapter bound to sheetRef, since a fallback with no adapter leaves this case unhealed. Tests: adapterRef.test.tsx ('closing' ends the sheet; 'hidden' still no-op and a switch-parked sheet is still restored) and a host.test.tsx case that throws mid-close.

[arekkubaczkowski]

When getSheetRef(id) misses, the boundary disappears silently — and permanently.

SheetWrapper && sheetRef ? … : content means a registry miss renders the sheet unwrapped, so a throw in its body propagates past the host and unmounts the group. That is the exact failure the prop exists to prevent, and it happens with no warning.

I checked whether it can recover, and it cannot. The React Compiler caches sheetRef on [id, usePortal], so a later setSheetRef plus any number of re-renders (updateParams, markOpen) never re-reads the map — I confirmed this with a throwaway test where the wrapper was never called once, for the item's whole lifetime.

Suggestion: widen SheetWrapperProps['sheetRef'] to SheetRef | undefined and render the wrapper unconditionally in the non-portal branch. At worst the fallback loses its ref; the boundary itself should never be conditional on registry state.

[VadymBezpalko]

Agreed on the mechanism and the permanence. I could not find a path to the miss through useBottomSheetManager().open() — the ref is registered in the same synchronous call — only through the @internal store, but a safety net conditional on registry state is wrong regardless.

sheetRef is now SheetRef | undefined, the wrapper renders unconditionally in the inline branch, and types.md mirrors the type. One addition your diff needs: useAdapterRef's parameter widens to ForwardedRef<SheetAdapterRef> | undefined too, otherwise the wrapper shape in host.test.tsx (and every wrapper following custom-adapters.md) stops typechecking. No extra __DEV__ warning: with the wrapper unconditional there is no observable state left to warn about.

[arekkubaczkowski]

This comment is inverted relative to the code, and the invariant it claims is not the one that holds.

In useBottomSheetManager.tsx the order is the opposite: storeOpen(...) runs first, and setSheetRef(id, ref) only after the result.opened check — deliberately, per the comment there about not leaking refs on a rejected open.

The read is safe today because React defers the useSyncExternalStore re-render, not because of any ordering inside open(). The CLAUDE.md paragraph states this correctly; this comment does not, and someone tidying the ordering later will believe the invariant is already guaranteed when it is not.

The cheap fix is to make it true. Registering before the store write and reclaiming on rejection keeps the no-leak property and turns the guarantee into actual ordering:

+    // Registered before the store write: QueueItem reads the map during its
+    // first render, and the store write is what schedules that render.
+    setSheetRef(id, ref);
+
     const result = storeOpen({ kind: 'inline', id, groupId, content: contentWithRef, ... }, options.mode);

-    // Registered only after the store accepts the sheet. [...]
+    // The map is module-global and otherwise only cleaned up by QueueItem's
+    // unmount, so a rejected open must reclaim its own entry — inline IDs are
+    // random, so every rejected call would leak one.
     if (!result.opened) {
+      cleanupSheetRef(id);
       return null;
     }
-
-    setSheetRef(id, ref);

I ran this against the full suite on this branch: 131/131 green, including registries.test.ts, which is the one asserting nothing leaks into the map.

[VadymBezpalko]

The comment was inverted and CLAUDE.md had the correct statement; rewritten, plus a test that opens through the public open() and asserts the wrapper's first render has the ref.

Your reorder as written regresses two cases the suite does not assert: a rejected re-open of a live explicit id (already-active) does write-then-delete and strips the mounted sheet's ref, and a group-busy rejection of a persistent id strips its mount-registered ref permanently, because BottomSheetPersistent registers only inside if (!sheetExists). hooks.test.tsx covers exactly that scenario and never looks at the map, which is why 131/131 stayed green. Took the reorder together with the C5 change (const existing = getSheetRef(id); const ref = existing ?? createRef()), reclaiming with if (!existing) cleanupSheetRef(id), and added the map assertion to that test.

[arekkubaczkowski]

A non-module-scope SheetWrapper remounts every inline sheet on every host render, and the new catch-up then restarts the open animation each time.

The constraint lives only in this JSDoc, and <BottomSheetHost SheetWrapper={(p) => <MyBoundary {...p} />} /> is the shape people will reach for first. BottomSheetHost is not memoized, so any parent re-render produces a new component type, QueueItem's memo compare fails, and React unmounts and remounts the whole inline subtree — the adapter loses its state and its imperative handle, and the boundary loses its failed state.

The catch-up compounds it: the freshly mounted adapter sees status === 'open' and calls expand() again. In CustomModalAdapter that resets progress to 0, which drives animatedIndex to -1 (blanking the shared backdrop) and re-runs the open animation — on every app render.

The library already warns in __DEV__ for smaller things (rejected opens, a missing ref in driveSheetRef). A __DEV__ warning when the SheetWrapper identity changes between renders would be cheap insurance for a footgun this prop newly introduces.

[VadymBezpalko]

The remount chain is as you describe, with two precisions. The trigger is a re-render of the host's parent (a store-driven host re-render keeps the same prop object and does not remount), and under React Compiler a capture-free inline arrow is outlined to module scope — but a derived prop (flag ? Boundary : undefined, or swapping two module-scope wrappers) remounts the open sheet even there, so the warning is worth having.

Added it, comparing the previous value rather than skipping the first effect run (the skip-first shape warns falsely under StrictMode's effect replay), and worded the JSDoc and components.md around keeping the prop value stable, not just module scope. One interaction with C1: a remount that lands mid-close now ends the sheet in the store instead of animating it out.

[arekkubaczkowski]

On the acknowledged same-tick re-open edge — one detail worth adding to the reasoning, since it is what makes the edge sticky rather than momentary.

Because the item keeps the same key, it never unmounts, so cleanupSheetRef does not run, and the compiler's cache on [id, usePortal] means the memoized sheetRef keeps the previous ref object for as long as the item lives — while open() has already written the new one. The content mounts against the new ref and the wrapper holds the old one, so a throw binds the fallback to a ref nobody reads; driveSheetRef then exhausts its 10 frames and force-closes with no animation.

Fine to ship as documented, but two options that would remove the class rather than describe it: key QueueItem on something that changes per open, or route the ref through the store instead of reading the module registry during render. I agree a fourth 'use no memo' is the wrong trade here.

[VadymBezpalko]

Confirmed end to end, and the writer line has a worse, pre-existing victim: manager.open(x, { id }) against a mounted BottomSheetPersistent id is accepted by the store (keepMounted+hidden passes the guard; the merge keeps usePortal and drops the content) and setSheetRef then clobbers the persistent sheet's own ref, so driveSheetRef burns its frames on a ref whose .current is null and useBottomSheetControl's !getSheetRef(id) guard refuses to restore it — permanent, and identical on main. That store.open silently discards inline content aimed at a keepMounted id is probably worth a separate issue as well.

Rather than a per-open key (the outgoing QueueItem's cleanup would delete the incoming item's registry entries by id) or routing the ref through the store, applied the guard useBottomSheetControl already has: const ref = getSheetRef(id) ?? React.createRef() in the manager. One registered id then maps to one ref object by construction, so the cached read cannot go stale. Tests cover the same-tick re-open and the persistent case.

[arekkubaczkowski]

The demo fallback flashes the shared backdrop off, then paints a second dim on top of it.

CustomModalAdapter drives animatedIndex off its own progress (animatedIndex.set(progress.value - 1) in a useDerivedValue), and progress starts at 0 — so the instant the fallback mounts, animatedIndex is -1. BottomSheetBackdrop is faded purely by animatedIndex, so the manager's backdrop snaps from fully opaque to transparent on the frame the crash lands and then fades back in.

Separately, styles.overlay gives the fallback its own full-screen rgba(0, 0, 0, 0.6), so once it settles the user sees the manager backdrop and the fallback's own overlay stacked — which is what pitfall 12 in CLAUDE.md warns adapters off doing.

Since the docs point at this file as the reference implementation, I would drop the overlay colour from contentContainerStyle and, if the flash is not worth fixing here, at least note the animatedIndex reset so third-party fallbacks do not inherit it.

[VadymBezpalko]

Both halves confirmed; the flash is a full ~300 ms re-fade rather than a frame, because the backdrop is QueueItem's sibling and follows the rewound animatedIndex. Dropped the colour from the example's contentContainerStyle — it was lifted from ModalSheets.tsx, and the same 0.6 overlay sits over the manager scrim in three example files on main, which I left for a separate sweep.

The docs note went next to the fallback snippet in components.md (that snippet is what app developers copy; custom-adapters.md cross-references it), phrased for an open sheet since the 'closing' catch-up makes the rewind correct there, plus a clause that an unmounting adapter's backdrop cleanup re-enables the manager scrim for a backdrop={false} sheet. Seeding CustomModalAdapter from status stays a follow-up: seeded unconditionally it leaves a normal open stuck at 'opening', and it fixes one adapter of three.

[VadymBezpalko]

Pushed six commits addressing all six threads, in dependency order C5 → C3 → C2 → C1 → C4 → C6 (details in each thread). Public API change only in the C2 commit: SheetWrapperProps.sheetRef and useAdapterRef's parameter accept undefined. Behaviour change in the C1 commit: an adapter mounting under 'closing' ends the sheet in the store. Description updated; yarn test / typecheck / lint / prepare green locally.

[VadymBezpalko]

@arekkubaczkowski all six threads are addressed and pushed; head is now c3fc838 (one more commit on top of the six trims the new comments and the identity-warning test harness, no behaviour change). Ready for another look.