elastic / elastic/docs-builder

Add a shared cleanup hook for JS observers on htmx swap

Open
#3,631 0 comments 0 reactions 0 assignees View on GitHub
needs triage tech-debt
Dominant language
C#
Stars
24
Forks
44
Avg merge
1d 7h
Merged PRs (30d)
146

Description

## 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` 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` 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 #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:

```ts
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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.