Skip to content

Dispose PlanetariumSkyNode child subtree to release label subscriptions.#4

Merged
veillette merged 1 commit into
mainfrom
claude/property-disposal-encapsulation-l71zy8
Jul 21, 2026
Merged

Dispose PlanetariumSkyNode child subtree to release label subscriptions.#4
veillette merged 1 commit into
mainfrom
claude/property-disposal-encapsulation-l71zy8

Conversation

@veillette

Copy link
Copy Markdown
Contributor

Node.dispose() only detaches children (removeAllChildren); it does not
dispose the child subtree. PlanetariumSkyNode.dispose() delegated to
super.dispose() alone, so its label Text nodes (cardinals, star/planet
names) and CelestialLinesNode's derived measure Properties stayed
subscribed to the shared StringManager and model Properties after the sky
node was torn down. A listener-count probe showed three StringManager
string Properties (measureSeparation, sun, north) gaining one listener per
create/dispose cycle and never releasing them.

  • PlanetariumSkyNode.dispose(): after detaching the redraw lazyLinks,
    dispose each child tree (the disposeSubtree walk, without re-entering the
    override) so those subscriptions are released. Corrects the dispose
    comment, which claimed super.dispose() recursively disposed children.
  • CelestialLinesNode: hold references to the measure label's backing
    DerivedProperty and PatternStringProperty and dispose them (label first,
    so it stops forwarding) in a new dispose() override; Node.dispose() never
    touched these standalone Properties.
  • memory-leak.test.ts: lock the fix with a WeakRef regression that fails if
    a cardinal label Text stays retained by the StringManager singleton after
    the sky node is disposed.

Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_013jBxyCRbyXXCozMaZhqvrd

Node.dispose() only detaches children (removeAllChildren); it does not
dispose the child subtree. PlanetariumSkyNode.dispose() delegated to
super.dispose() alone, so its label Text nodes (cardinals, star/planet
names) and CelestialLinesNode's derived measure Properties stayed
subscribed to the shared StringManager and model Properties after the sky
node was torn down. A listener-count probe showed three StringManager
string Properties (measureSeparation, sun, north) gaining one listener per
create/dispose cycle and never releasing them.

- PlanetariumSkyNode.dispose(): after detaching the redraw lazyLinks,
  dispose each child tree (the disposeSubtree walk, without re-entering the
  override) so those subscriptions are released. Corrects the dispose
  comment, which claimed super.dispose() recursively disposed children.
- CelestialLinesNode: hold references to the measure label's backing
  DerivedProperty and PatternStringProperty and dispose them (label first,
  so it stops forwarding) in a new dispose() override; Node.dispose() never
  touched these standalone Properties.
- memory-leak.test.ts: lock the fix with a WeakRef regression that fails if
  a cardinal label Text stays retained by the StringManager singleton after
  the sky node is disposed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013jBxyCRbyXXCozMaZhqvrd
@cursor

cursor Bot commented Jul 21, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@github-actions

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@veillette
veillette merged commit 7627c3d into main Jul 21, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants