[haxcms-elements] Dispose theme wiring watchdog autorun and fix constructor __disposer orphaning (haxtheweb/issues#3106) - #836
Merged
Merged
Conversation
…heme disconnect and re-establish it on reconnect (haxtheweb/issues#3106) The wiring instance created by the HAXCMSTheme mixin constructor pushed its editor-reconnect watchdog autorun onto its own __disposer array, which nothing ever disposed: every removed theme leaked a live autorun still watching store.jwt / store.themeElement / store.isLoggedIn (one zombie per theme swap). Extract the watchdog into a guard-flagged establishDisposers() method, add disposeDisposers() to the wiring class, dispose it from the mixin's disconnectedCallback, and re-establish it from connectedCallback so reconnects keep editor wiring. Also push a trayStatus store-sync autorun on every connect so a reconnected theme keeps syncing trayStatus, previously the one constructor-time sync that was never re-established. Co-Authored-By: Warp <agent@warp.dev>
…ant dispose loop in haxcms-slide-theme (haxtheweb/issues#3106) The constructor reassigned __disposer = [] right after super(), orphaning the three constructor-time autoruns pushed by HAXCMSLitElementTheme (editMode/trayStatus/activeItemContent) so removal never disposed them. The disconnectedCallback override also carried a manual dispose loop that duplicated (and double-ran) the mixin's disposeDisposers. Co-Authored-By: Warp <agent@warp.dev>
…autoruns (haxtheweb/issues#3106) Every terrible theme constructor reassigned __disposer = [] right after super(), orphaning the six constructor-time autoruns pushed by the class chain (3 from HAXCMSLitElementTheme + 2 from HAXCMSThemeParts + 1 from HAXCMSRememberRoute) so removal never disposed them (zombie autoruns on every removed theme, one per swap). Co-Authored-By: Warp <agent@warp.dev>
…utoruns (haxtheweb/issues#3106) The constructor reassigned __disposer = [] right after super(), orphaning the three constructor-time autoruns pushed by HAXCMSLitElementTheme (editMode/trayStatus/activeItemContent) so removal never disposed them. The regenerated manifest drops the __disposer field CEM inferred from that assignment. Co-Authored-By: Warp <agent@warp.dev>
…eweb/issues#3106) The manual dispose loop in disconnectedCallback duplicated the shared HAXCMSTheme mixin's disposeDisposers(), which now also disposes the wiring instance's watchdog autorun. Co-Authored-By: Warp <agent@warp.dev>
…axtheweb/issues#3106) The manual dispose loop in disconnectedCallback duplicated the shared HAXCMSTheme mixin's disposeDisposers(), which now also disposes the wiring instance's watchdog autorun; the keydown listener removal stays. Co-Authored-By: Warp <agent@warp.dev>
…axtheweb/issues#3106) The manual dispose loop in disconnectedCallback duplicated the shared HAXCMSTheme mixin's disposeDisposers(), which now also disposes the wiring instance's watchdog autorun. Co-Authored-By: Warp <agent@warp.dev>
…axtheweb/issues#3106) The manual dispose loop in disconnectedCallback duplicated the shared HAXCMSTheme mixin's disposeDisposers(), which now also disposes the wiring instance's watchdog autorun. Co-Authored-By: Warp <agent@warp.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The two unrelated HAX browser styling changes should be removed or split into a documented, tested change.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Fixes HAXCMS theme teardown leaks and reconnect synchronization.
Changes:
- Disposes and re-establishes theme wiring watchdogs.
- Preserves inherited autorun disposers and removes redundant cleanup.
- Adds lifecycle tests; also includes two unrelated heading-style changes.
| File | Description |
|---|---|
elements/hax-body/lib/hax-gizmo-browser.js |
Adds unrelated heading layout styling. |
elements/hax-body/lib/hax-stax-browser.js |
Adds unrelated heading layout styling. |
elements/haxcms-elements/lib/core/HAXCMSThemeWiring.js |
Manages watchdog and reconnect disposers. |
elements/haxcms-elements/lib/core/themes/haxcms-slide-theme.js |
Preserves inherited disposers. |
elements/haxcms-elements/test/HAXCMSLitElementTheme.test.js |
Tests tray-status reconnection. |
elements/haxcms-elements/test/HAXCMSThemeWiring.test.js |
Tests watchdog lifecycle. |
elements/haxcms-elements/test/haxcms-slide-theme.test.js |
Tests slide-theme disposal. |
elements/haxma-theme/haxma-theme.js |
Removes redundant cleanup. |
elements/haxma-theme/test/haxma-theme.test.js |
Tests complete disposal. |
elements/link-card-theme/link-card-theme.js |
Removes redundant cleanup. |
elements/link-card-theme/test/link-card-theme.test.js |
Verifies wiring disposal. |
elements/outline-player/custom-elements.json |
Regenerates component metadata. |
elements/outline-player/outline-player.js |
Preserves inherited disposers. |
elements/outline-player/test/outline-player.test.js |
Tests retained and disposed autoruns. |
elements/spacebook-theme/spacebook-theme.js |
Delegates cleanup while retaining listener removal. |
elements/spacebook-theme/test/spacebook-theme.test.js |
Tests teardown behavior. |
elements/terrible-themes/lib/terrible-best-themes.js |
Preserves constructor-chain disposers. |
elements/terrible-themes/lib/terrible-outlet-themes.js |
Preserves constructor-chain disposers. |
elements/terrible-themes/lib/terrible-productionz-themes.js |
Preserves constructor-chain disposers. |
elements/terrible-themes/lib/terrible-resume-themes.js |
Preserves constructor-chain disposers. |
elements/terrible-themes/terrible-themes.js |
Preserves constructor-chain disposers. |
elements/terrible-themes/test/store-driven-suite.test.js |
Tests all theme disposal paths. |
elements/twenty-six-theme/test/twenty-six-theme.test.js |
Tests shared cleanup behavior. |
elements/twenty-six-theme/twenty-six-theme.js |
Removes redundant cleanup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| a11y-collapse::part(heading) { | ||
| margin: var(--ddd-spacing-2) 0; | ||
| display: block; |
| } | ||
| a11y-collapse::part(heading) { | ||
| margin: var(--ddd-spacing-2) 0; | ||
| display: block; |
This branch had an error being deployed
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary
Fixes the theme teardown autorun leak family from haxtheweb/issues#3106 (deferred from the #3102 fix swarm; exemplar fix PR #832).
HAXCMSThemeWiringwatchdog disposal (core): the wiring instance created per theme pushed its editor-reconnect watchdog autorun onto its own__disposerarray, which nothing ever disposed — every removed theme leaked a live autorun still watchingstore.jwt/store.themeElement/store.isLoggedIn(one zombie per theme swap). The watchdog is now extracted into a guard-flaggedestablishDisposers()method,disposeDisposers()is added to the wiring class, the mixin'sdisconnectedCallbackdisposes it, andconnectedCallbackre-establishes it so reconnects keep editor wiring. No behavior change on the first-connect path (the guard prevents a double-push).connectedCallbackre-pushes activeItemContent / editMode / isLoggedIn / manifest / location on every connect but nevertrayStatus, so a reconnected theme stopped syncing it. AtrayStatusstore-sync autorun is now pushed on every connect.this.__disposer = []reassignment (the simple-blog #18 pattern) dropped in:haxcms-slide-theme— orphaned the 3 constructor-time autoruns fromHAXCMSLitElementTheme; its redundant manual dispose loop (which double-disposed before the mixin'sdisposeDisposers) is also removedterrible-themes+ the 4 lib variants — each orphaned 6 constructor-time autoruns (3HAXCMSLitElementTheme+ 2HAXCMSThemeParts+ 1HAXCMSRememberRoute)outline-player— orphaned 3; the regenerated manifest drops the__disposerfield CEM had inferred from that assignmentdisconnectedCallbacksimplified in haxma-theme, spacebook-theme (keydown listener removal kept), twenty-six-theme, and link-card-theme — the shared mixin'sdisposeDisposers()already handles disposal, and now the wiring watchdog too.Tests
Mirrors the PR #832 patterns from
simple-blog/test/simple-blog-suite.test.js: retain-count, full-disposal-on-disconnect, and no-zombie-reaction assertions for every touched theme; wiring lifecycle tests (establish once / dispose on theme disconnect / re-establish on reconnect) plus trayStatus reconnect tests; the directly instantiated wiring objects in the existing wiring tests are now disposed inafterEachso the suite stops leaking watchdog autoruns itself.Validation
haxcms-slide-theme.test.js), terrible-themes, outline-player, haxma-theme, spacebook-theme, twenty-six-theme, link-card-theme — all greenyarn run build(prettier + CEM) run for every touched package;custom-elements.jsonchanges are tool-generated, not hand-editedFollow-ups found during the audit (not addressed here)
haxcms-site-builder.js:820-849— two bareautorun()calls in the constructor are never pushed to__disposer/ never disposed, and itsconnectedCallbackpushes autoruns insidesetTimeout(0)that can land after a fast disconnectLTIResizingMixin.js:43— a bare LMS frame-resizeautorun()is never disposedFixes haxtheweb/issues#3106
Co-Authored-By: Warp agent@warp.dev