Skip to content

fix(website): remove Wistia players on unmount - #20056

Draft
posthog[bot] wants to merge 1 commit into
masterfrom
posthog-self-driving/fixwebsite-remove-wistia-players-on-a8f704
Draft

fix(website): remove Wistia players on unmount#20056
posthog[bot] wants to merge 1 commit into
masterfrom
posthog-self-driving/fixwebsite-remove-wistia-players-on-a8f704

Conversation

@posthog

@posthog posthog Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Changes

Our three hand-rolled Wistia wrappers tore the player's DOM out on unmount, but never told the player to stop. Wistia keeps a player registered and its timers running until you call remove() on it. The result is a player that keeps firing events at nodes that no longer exist, and a vendor event relay that throws when it dereferences internals that are gone.

  • Who is hurt: visitors to pages with an embedded demo video get uncaught TypeErrors from the vendor script. Every visit that leaves such a page also leaks a live player that keeps running for the rest of the session.
  • The mechanism: teardown used containerRef.current.innerHTML = '' (WistiaVideo, MediaPlayer) or did nothing at all. WistiaCustomPlayer had a cleanup that only wrote to the console and let the player persist on purpose.
  • The fix: each wrapper now calls the player's own remove() before it clears its container. innerHTML = '' is no longer used as a lifecycle tool.
  • A second defect found while testing: a _wq entry stays registered against its media id, so an entry from an earlier mount also receives a later mount's player. Each onReady now checks that the player belongs to its own embed. Without this check, an unmounted player's callback destroys a live video.
  • New src/lib/wistia.ts holds the teardown and swallows a vendor error, so unmount always finishes.

Mechanical: an import line per component, and a test:wistia script that follows the existing test:navs and test:sdk-references pattern.

Not a visual change. Nothing renders differently. The change only affects what happens after React unmounts a player, so there are no screenshots.

🗺️ PR tour

1. The shared teardown — src/lib/wistia.ts (new)

Start here. One function. It calls the player's remove() and swallows an error, because a teardown that throws is as bad as no teardown. The comment records why detaching the container is not enough.

// Wistia keeps a player registered and its timers running until remove() is called.
// A wrapper that only detaches the container leaves the vendor script firing events
// at DOM nodes that no longer exist, which throws inside Wistia's own event relay.
// remove() takes the embed element with it.
export function removeWistiaPlayer(player: { remove?: () => void } | null | undefined): void {
if (!player) return
try {
player.remove()
} catch {
// The player can already be gone. Teardown must not throw.
}
}

2. WistiaVideo — the wrapper in the signal

This is the component the homepage demo uses. Cleanup unbinds the end handler as before, then removes the player, then empties the container.

replaceChildren() replaces innerHTML = ''. It covers an embed that never became a player. The container is already held in containerRef, so no new ref is needed.

if (playerRef.current && endHandlerRef.current) {
try {
playerRef.current.unbind('end', endHandlerRef.current)
} catch (e) {
// Ignore
}
}
endHandlerRef.current = null
removeWistiaPlayer(playerRef.current)
playerRef.current = null
// Covers an embed that never became a player.
containerRef.current?.replaceChildren()
}

3. WistiaCustomPlayer — the persist-forever no-op

The old cleanup logged a message and kept the player alive on purpose. The comment said this stopped re-initialization on tab focus changes, but the effect only depends on mediaId, and React does not re-run effects on focus. The guard above it already skips an existing player.

The new cleanup removes the player, clears the ref, resets isReady, and drops an embed that never initialized.

}
return () => {
cancelled = true
removeWistiaPlayer(playerRef.current)
playerRef.current = null
setIsReady(false)
// Covers an embed that never became a player.
embedDiv?.remove()
}

4. The onReady identity guard — both _wq wrappers

Same three lines in WistiaCustomPlayer and MediaPlayer. A stale entry must not touch a player it does not own. See the reviewer's guide for the measurement that made this necessary.

// A _wq entry stays registered against its media id, so an entry from an
// earlier mount also receives a later mount's player. Act on our embed only.
if (video.container !== embedDiv) return
// Wistia can hand the player back after the component unmounts.
if (cancelled) {
removeWistiaPlayer(video)
return
}
playerRef.current = video

5. MediaPlayer — the Wistia branch had no cleanup at all

The Wistia branch returned nothing, so a player survived both unmount and every change of videoId, source, startTime, or borderRadius.

Two things here beyond the common pattern. The cleanup sets the player in state to null, because a 250 ms interval polls player.time() and must stop. It also drops the local player reference, because Wistia retains the pushed _wq entry and its onReady closure shares this scope. embedDiv stays, since the callback still needs it to recognize its own embed.

return () => {
cancelled = true
removeWistiaPlayer(player)
// Wistia retains the pushed _wq entry, and its onReady closure shares this
// scope. Drop the player so the entry does not pin it for the page's lifetime.
// embedDiv stays: the callback still needs it to recognize its own embed.
player = null
// The progress interval polls player.time(). Drop the handle so it stops.
setPlayerState((prev) => ({ ...prev, player: null }))
// Covers an embed that never became a player.
embedDiv?.remove()
}

6. Tests and script — src/lib/wistia.test.ts, package.json

Three cases on the helper: it delegates, it stays quiet with no player, and it swallows a vendor error. Run with pnpm test:wistia.

🔍 Reviewer's guide

Testing done

Check Command / method Result
Formatting npx prettier --check on the 5 changed files All files match Prettier style
Types npx tsc --noEmit No errors in src/. The 267 reported errors are all pre-existing node_modules type-parse noise
Lint npx eslint on the 4 changed components 0 errors. 47 warnings, all pre-existing any and unused-var warnings
Unit test pnpm test:wistia 3 pass, 0 fail
Dev server pnpm start, loaded / and /ai Pages render. No new console errors — the SVG width and DOM-nesting warnings are present without this change too
Bug reproduced On /ai, client-side navigation away (a real React unmount), before the fix Wistia.api.all().length stays at 2 while .wistia_embed count drops to 0 — players alive, DOM gone
Bug fixed Same page, same navigation, after the fix Players drop to 0. Measured again at 15 s, 30 s, 60 s and 90 s: still 0
Teardown is quiet Same page reduced to a single player, then remove() 0 uncaught errors
Vendor mechanism Isolated page loading the real E-v1.js, video playing, then teardown innerHTML = '' leaves a timechange tick firing after teardown. remove() leaves none
_wq reuse Isolated page: push an entry, drop its embed, remount the same media id The stale entry's onReady receives the new player (video.container is not its own embed). This is what the identity guard prevents

Not tested: the production build (pnpm build) and the Vercel preview. Both need a full build, which exceeds this environment. iOS Safari, the browser in the original signal, was not available — the fix targets the mechanism, which was measured on Chromium.

Where the risk is

  • The identity guard is the highest-value hunk to review. Without it, this change would be a regression: a stale _wq entry's onReady would call remove() on a newly mounted player and leave a dead video. This is measured, not theoretical.
  • One known Wistia limit, unchanged by choice. When two embeds of the same media id are on a page at once, remove() on one nulls state the other still uses, and the vendor throws. This shows up on /ai in dev, which mounts Demos twice. With one player — what the homepage renders — teardown is silent. Detaching the DOM without remove() throws in the same place, so this is a vendor constraint rather than something this change introduces.
  • WistiaCustomPlayer reverses a deliberate decision. If the player was kept alive for a reason not written in the comment, this is the hunk that would show it.

Deliberately left out

  • The YouTube branch of MediaPlayer has the same missing teardown: its YT.Player is never captured and never destroyed. It is a different vendor and not part of the reported errors, so it stays out of this diff.
  • The !window.Wistia script loader is still copy-pasted in three places and does not dedupe an in-flight load, so two wrappers mounting together can each append the script.
  • Two timers started inside WistiaCustomPlayer's onReady (captions polling) still outlive unmount. Their bodies swallow errors, so they leak quietly rather than throw.
  • Moving these wrappers onto @wistia/wistia-player-react, which WistiaEmbed already uses, would delete this whole class of bug. WistiaVideo is the natural first candidate. That is a rewrite per component, not this PR.

Checklist

  • I've read the docs and/or content style guides. — not a docs or content change
  • Words are spelled using American English
  • Use relative URLs for internal links — no new links added
  • I've checked the pages added or changed in the Vercel preview build — see "Not tested" above
  • If I moved a page, I added a redirect in vercel.json — no pages moved

Created with PostHog Desktop from this inbox report.

The three hand-rolled Wistia wrappers detached the player's DOM without telling
the player to stop. The vendor script kept the player registered and its timers
running against nodes that no longer existed, so its event relay threw.

Each wrapper now calls the player's own remove() before it clears the container.
WistiaCustomPlayer loses its no-op cleanup. A shared removeWistiaPlayer() helper
holds the teardown and swallows a vendor error so unmount always finishes.

The onReady callbacks also check that the ready player belongs to their own
embed. A _wq entry stays registered against its media id, so an entry from an
earlier mount also receives a later mount's player.

Generated-By: PostHog Desktop
Task-Id: c6cd30ed-97b5-4688-ae54-0b60191c98b0
@github-actions

github-actions Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Deploy preview

Status Details Updated (UTC)
🟢 Ready View preview Sep 10, 2026 01:42AM

@github-actions

Copy link
Copy Markdown
Contributor

Bundle report

Total JS (gzip)

8.86 MiB (+0.3 KiB / +0.0%)

Eager graph (modules shipped in each entrypoint's initial chunks)

Entrypoint Eager size Budget Modules
app 18.47 MiB (+1.8 KiB / +0.0%) report-only 2055
Largest modules in the app closure
Module Size
./src/data/mcp-tools.json 1119.4 KiB
css ./node_modules/.pnpm/css-loader@5.2.7_webpack@5.101.3/node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[8].oneOf[1].use[1]!./node_modules/.pnpm/postcss-loader@4.3.0_postcss@8.5.6_webpack@5.101.3/node_modules/postcss-loader/dist/cjs.js??ruleSet[1].rules[8].oneOf[1].use[2]!./src/styles/global.css 761.0 KiB
./src/components/Stickers/Stickers.tsx 696.4 KiB
./node_modules/.pnpm/@radix-ui+react-icons@1.3.2_react@18.3.1/node_modules/@radix-ui/react-icons/dist/react-icons.esm.js 481.4 KiB
./node_modules/.pnpm/@posthog+brand@0.8.0_react@18.3.1/node_modules/@posthog/brand/dist/generated/hoggies/svg/x-ray.mjs 480.8 KiB
./node_modules/.pnpm/rehype-raw@7.0.0/node_modules/rehype-raw/lib/index.js + 29 modules 395.1 KiB
./node_modules/.pnpm/@posthog+brand@0.8.0_react@18.3.1/node_modules/@posthog/brand/dist/generated/hoggies/svg/im-the-driver.mjs 385.7 KiB
./src/hooks/useCustomers.tsx + 55 modules 370.0 KiB
./node_modules/.pnpm/@posthog+icons@0.36.6_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js 354.8 KiB
./node_modules/.pnpm/react-markdown@8.0.7_@types+react@16.14.66_react@18.3.1/node_modules/react-markdown/lib/react-markdown.js + 88 modules 351.4 KiB
./src/components/ProductComparisonTable/index.tsx + 126 modules 302.5 KiB
./node_modules/.pnpm/cloudinary-core@2.14.0_lodash@4.17.21/node_modules/cloudinary-core/cloudinary-core.js 281.9 KiB
./node_modules/.pnpm/@posthog+brand@0.8.0_react@18.3.1/node_modules/@posthog/brand/dist/generated/hoggies/svg/doll-house.mjs 281.7 KiB
./node_modules/.pnpm/@posthog+brand@0.8.0_react@18.3.1/node_modules/@posthog/brand/dist/generated/hoggies/svg/director.mjs 275.6 KiB
./src/components/SearchUI/index.tsx + 87 modules 273.7 KiB

Eager-graph budgets are report-only until a baseline is established. Sizes are gzip of public/**/*.js; eager size is webpack module source bytes for the modules actually shipped in the entrypoint's initial chunks (post-tree-shake).

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.

0 participants