fix(inspector): release animation and texture state on teardown - #136
Merged
Conversation
Two retention paths, both only reachable with the inspector enabled: - Animations still running when their node is destroyed never emit `stopped`, so their entry — and the IAnimationController it holds — stayed in the static `activeAnimations` map for the lifetime of the page. Destroy now ends them as `cancelled`, which also keeps `getAnimationStats()` consistent instead of silently dropping them. - `textureMetrics` was a strong `Map<Texture, TextureMetrics>` on an object that lives as long as the renderer, and entries were only dropped for the texture a node happened to hold at destroy time. `setupTextureListeners` detaches the old texture's listeners on every swap but never dropped its metrics entry, so every texture ever swapped off a node stayed reachable, bitmap included. `WeakMap` instead — nothing iterates it, so the API is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Two retention paths in
Inspector.ts, both only reachable with the inspector enabled.Originally found while reviewing lightning-js/renderer#846. Of the four issues in that review, this repo has already fixed two —
destroyNode(node)uses the mirror div stored on the node instead of a document-globalgetElementById, and theparentbranch ofupdateNodePropertyalready falls back withparentNode.div ?? this.root. The two below are still present.1.
activeAnimationsnever releases animations killed by destroyInspector.activeAnimationsis astaticMap holding theIAnimationController. Entries are only removed bytrackAnimationEnd, which runs onstop/restore/waitUntilStoppedor the controller'sstoppedevent. A node destroyed mid-animation fires none of those, so the entry and the controller it holds are retained by a static root for the lifetime of the page.Destroy now ends those animations as
cancelledrather than deleting them outright, sogetAnimationStats()andanimationHistorystay consistent —restore()already recordscancelledfor the same situation.2.
textureMetricspinned every texture ever usedIt was a strong
Map<Texture, TextureMetrics>on an object that lives as long as the renderer.setupTextureListenersdetaches the old texture's listeners on every swap but never dropped its metrics entry, and destroy only cleaned the texture a node happened to hold at that moment. Every texture ever swapped off a node stayed reachable, bitmap included.Now a
WeakMap. Nothing iterates it — onlyhas/get/set/delete— so the API is unchanged.Testing
pnpm testgreen (360 tests, 42 files),tsc --noEmitclean, prettier clean, no new eslint warnings (the two reported onInspector.tsare onmaintoo).No regression test here: this repo has no DOM test environment, and the inspector needs one to instantiate. The equivalent test — asserting
getActiveAnimations()no longer lists a destroyed node — is in lightning-js/renderer#847, wherehappy-domis already a devDependency. Happy to addhappy-domhere and port it over if that's wanted.🤖 Generated with Claude Code