Skip to content

Add a shared cleanup hook for JS observers on htmx swap #3631

Description

@reakaleek

Why

htmx swaps out DOM content on navigation without releasing JS-side references to the removed elements. Anything that holds a reference to a DOM node (a ResizeObserver, IntersectionObserver, MutationObserver, or a closure) keeps that node reachable from JS forever unless something explicitly detects "this element left the document" and cleans up.

Two places in the frontend already hit this and each solved it with its own one-off strategy instead of a shared mechanism:

  • Assets/table.ts's overflowObserver (ResizeObserver) tracks a Set<HTMLElement> and sweeps it for .isConnected on every htmx:load.
  • Assets/main.ts's ctaImpressionObserver (IntersectionObserver) self-unobserves after firing once, which only works because its semantics happen to be "fire once, then done" — an off-screen CTA that never intersects before its page is swapped away stays observed forever.

Every future observer-based feature would otherwise need to reinvent this again. This issue tracks introducing a shared htmx:beforeCleanupElement-based hook that any observer can register against, and migrating the two existing implementations onto it.

What

See the task prompt below for full context and proposed approach.

Prompt for the implementing agent

Add a shared cleanup hook for JS observers that reference DOM elements htmx removes on swap, in docs-builder's frontend (src/Elastic.Documentation.Site/Assets/).

Background

This is an htmx-driven site: main.ts re-runs a list of init* functions on every htmx:load (navigation), but htmx swapping out old page content doesn't automatically release JS-side references to the removed elements. Any ResizeObserver/IntersectionObserver/MutationObserver (or closure) that holds a reference to a DOM node keeps it reachable from JS forever unless something explicitly detects "this element left the document" and cleans up — otherwise every navigation leaks that page's detached DOM subtree for the lifetime of the tab.

Two places in the codebase already hit this, and each solved it with its own one-off strategy:

  1. Assets/table.ts — a module-level overflowObserver (ResizeObserver) watches every markdown table wrapper for overflow. It currently tracks a Set<HTMLElement> of what it's watching, and on every initTable() call (which runs on htmx:load) loops the set checking .isConnected, calling .unobserve() and deleting from the set for anything no longer in the document. See observedWrappers, the cleanup loop at the top of initTable(), and overflowObserver itself.

  2. Assets/main.ts — a module-level ctaImpressionObserver (IntersectionObserver, declared around line 162, built in initCtaImpressions() around line 168) fires a cta_viewed event the first time a CTA link becomes visible, then calls observer.unobserve(entry.target) on itself inside the callback. This "self-drains" over time, but only works because the semantics happen to be "fire once, then done" — a CTA that's off-screen and never intersects before its page is swapped away stays observed forever. Same leak class as Configure Renovate #1, just a smaller/less-likely surface area.

Neither of these detects removal via htmx's own lifecycle; both independently reinvent "did my element go away" detection. htmx fires htmx:beforeCleanupElement on an element right before it internally tears it down during a swap — this is htmx's designated hook for "release anything you attached to this element," and the codebase already listens to sibling swap-lifecycle events for related purposes (htmx:removingHeadElement, htmx:oobBeforeSwap, both in main.ts), so this isn't introducing an unfamiliar pattern.

What to build

  1. A small shared utility (new file, e.g. Assets/element-cleanup.ts, or fold into main.ts if that fits the codebase's conventions better — check how other small shared helpers are organized first) exposing something like:

    const cleanupCallbacks = new Set<(el: Element) => void>()
    
    export function onElementCleanup(callback: (el: Element) => void): void {
        cleanupCallbacks.add(callback)
    }
    
    document.addEventListener('htmx:beforeCleanupElement', (e) => {
        for (const cb of cleanupCallbacks) cb(e.target as Element)
    })

    Treat this as a starting sketch, not a spec — adjust naming/shape to fit existing conventions (check how main.ts types htmx events, e.g. its local HtmxEvent alias, and reuse it).

  2. Migrate table.ts's overflowObserver to register via this hook instead of the manual Set + .isConnected sweep in initTable(). Confirm htmx:beforeCleanupElement actually fires per-element for the table wrappers this observer cares about (see verification section) before ripping out the existing tracked-Set fallback — if it doesn't fire reliably for every removal path this site uses (full-page swaps, oob-swaps, etc.), keep the existing mechanism and say so rather than swapping to something less correct.

  3. Migrate main.ts's ctaImpressionObserver to also register cleanup via the same hook (in addition to its existing self-unobserve-after-fire, or replacing it if the new hook alone is sufficient — your call once you've confirmed the event's actual firing behavior).

  4. Any future observer-based feature should be able to use onElementCleanup instead of reinventing this. Don't go looking for other places to retrofit beyond these two unless you find another clear instance while you're in there.

Verification

  • Confirm htmx:beforeCleanupElement actually fires for the elements you care about in this app's swap setup — add a temporary console.log and manually navigate between pages with tables/CTAs a few times in a running dev server (dotnet run --project src/tooling/docs-builder -- serve from repo root, then hit the built site) before trusting it for real.
  • For table.ts: navigate to a page with an overflowing table, navigate away, navigate back, and confirm (via a temporary breakpoint/log, or by tracking a counter) that a table wrapper removed by a swap actually gets .unobserve()'d — not just that the feature still visually works (it will keep working today even if cleanup is silently broken, since the visible symptom is a slow memory leak, not a functional bug).
  • For CTAs: confirm impressions still fire correctly (cta_viewed events) after the migration — check src/Elastic.Documentation.Site/Assets/telemetry/ for how impressions are asserted/tested if there's existing coverage.
  • Run npm run compile:check and npm run build in src/Elastic.Documentation.Site, and the repo's existing JS/TS test suite (check package.json for the test script) if either file has coverage.
  • This is a cross-cutting cleanup of already-shipped code, unrelated to any single feature — keep it as its own PR, not bundled with unrelated feature work.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions