fix: cancel DismissibleLayer afterSleep timer on destroy (#2080) - #2087
Open
dieison-depra wants to merge 2 commits into
Open
fix: cancel DismissibleLayer afterSleep timer on destroy (#2080)#2087dieison-depra wants to merge 2 commits into
dieison-depra wants to merge 2 commits into
Conversation
Prevents derived_inert warnings and stale document pointerdown listeners when a layer is torn down within the 1ms attach window (afterSleep). Closes the race described in huntabyte#2080.
🦋 Changeset detectedLatest commit: 99c2d8a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
|
Hit this too — confirmed independently in a Svelte 5 game project using Dialog. One interesting extra detail: the warnings genuinely compound over a long session — each subsequent click/focus event anywhere in the app seems to re-trigger every stale listener accumulated so far, so a single click later on can produce 15-20 warnings at once, not just one. Would be great to see #2087 merged - thanks for the fix! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Hi maintainers 👋
Thank you for Bits UI — we rely on it heavily for Dialog / Sheet / Popover surfaces in a Svelte 5 app, and we are grateful for the care that goes into the headless layer.
While working through browser-test noise on our side, we kept hitting Svelte’s
derived_inertwarning when floating layers unmounted quickly. That led us to #2080, which already describes the race clearly. We would like to offer a small, focused patch as a proposal — happy to iterate if you prefer a different shape.Root cause (what we validated)
In
DismissibleLayerState(use-dismissable-layer.svelte.ts), setup schedules:What we observed when a layer is destroyed within that ~1 ms window (common under vitest-browser cleanup, rapid navigation, or CPU contention):
onDestroyEffectruns whileunsubEventsis still a no-op (the real subscribe has not happened yet).afterSleeptimer is not cancelled.this.opts.ref.current→ Svelte emitsderived_inert.pointerdownlisteners can be attached to a dead layer and never cleaned up — aligning with the comment on DismissibleLayer: derived_inert warning — afterSleep(1) timer reads ref.current after the layer is destroyed #2080 about reopened dialogs self-closing ~10 ms later (staleonInteractOutside+ debounce).We confirmed stacks in a real SvelteKit + vitest-browser suite (
bits-ui@2.18.1) converging on:get current(svelte-toolbelt Box /refderived)#markResponsibleLayer/ the deferred attach pathpointerdownThis is the same family of teardown race already documented in #2080; we are not claiming a new root cause, only re-validating it end-to-end before suggesting a fix.
Proposed change
Minimal lifecycle hygiene, aligned with the suggested fix in #2080 and with patterns already used elsewhere in this repo (e.g. clearing timers in Select / Popover / Menu):
afterSleeptimer (pendingTimer).cleanup()andonDestroyEffect.destroyedflag on teardown and short-circuit ondestroyedbefore anythis.opts.ref.currentread (so JS never touches the destroyed derived).No public API changes. No behavior change for the happy path (layer lives longer than 1 ms).
Tests
Added two browser regressions under
tests/src/tests/dialog/dialog.browser.test.ts:console.warndoes not containderived_inertderived_inertLocal verification (after
pnpm build:packages):Result:
43 passed | 1 todo(full dialog browser file, Chromium).We also exercised the same patch against our app’s LookupModal / ResponsiveSheet vitest-browser suite (open → unmount, open → close → open) with zero
derived_inertwarnings after the change.Invitation / next steps
This is offered as a collaborative starting point, not a demand that the fix must land exactly this way. If you would rather:
we are very happy to follow your guidance and update the PR.
Closes #2080
Thank you for considering this, and for the excellent library 🙏