fix(replay): ignore autoplay mutations regardless of tag name case - #4837
Open
pauldambra wants to merge 3 commits into
Open
fix(replay): ignore autoplay mutations regardless of tag name case#4837pauldambra wants to merge 3 commits into
pauldambra wants to merge 3 commits into
Conversation
`ignoreAttribute()` compares the tag name against lowercase 'video'/'audio', but the attribute-mutation path passed `Element.tagName`, which is uppercase for HTML elements. Every `autoplay` toggle on a looping background video was therefore recorded as a mutation. Normalise the tag name once at the top of the attributes branch and use it for `ignoreAttribute`, `transformAttribute` and the iframe/input/dialog checks, and lower-case the attribute name inside `ignoreAttribute` so XML documents that preserve attribute case are covered too. Ported from upstream rrweb #1921 (fixes rrweb-io/rrweb#1916). Generated-By: PostHog Desktop Task-Id: 3596bd9c-f9bc-42dc-a43a-535c33ac6d9b
Contributor
Replay incident risk checkThis diff touches code involved in past incidents. This is a heads-up, not a verdict: read the matched sections of INCIDENTS.md and answer their review questions before merging. For a judgment on whether this diff has the same failure mode, run the |
Contributor
Contributor
|
Size Change: +396 B (0%) Total Size: 20.6 MB 📦 View Changed
ℹ️ View Unchanged
|
pauldambra
marked this pull request as ready for review
September 8, 2026 13:24
pauldambra
requested review from
TueHaulund,
arnohillen,
fasyy612 and
ksvat
and removed request for
a team
September 8, 2026 13:24
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Member
Author
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: router (cheap-first pass) + delegated reviewers (qa-team, paul-reviewer, xp-reviewer, security-audit as warranted) Verdict: ✅ APPROVE (round 2 @ 4c963c8)Re-review of the incremental commit Key findingsNone. Convergencen/a — a single reviewer ran (see below). Reviewer summaries
Worth recording for the archaeology: the empty-item problem the Codex thread found was real, not cosmetic. The attribute emission path in Previous rounds (1)round 1 @ d81152c — ✅ APPROVE: router reviewed the full 164-line diff with surrounding context, found nothing, delegated nothing; Automated by QA Swarm — not a human review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d81152caed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
When autoplay attribute mutations are ignored, we were still creating an empty item in the attributes array and never populating it. Move the item creation inside the !ignoreAttribute() check so we only create items when we're actually recording attributes. This prevents empty mutation events from being emitted on every observer flush. Generated-By: PostHog Desktop Task-Id: 3596bd9c-f9bc-42dc-a43a-535c33ac6d9b
Generated-By: PostHog Desktop Task-Id: 3596bd9c-f9bc-42dc-a43a-535c33ac6d9b
marandaneto
approved these changes
Sep 8, 2026
fasyy612
approved these changes
Sep 9, 2026
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Problem
ignoreAttribute()exists so thatautoplaytoggles on<video>/<audio>are not recorded — the replayer controls playback itself, so those mutations are pure noise. It compares the tag name against lowercase'video'/'audio':The initial-snapshot path passes a lowercased name (
getValidTagName), so it works there. The attribute-mutation path passedElement.tagName, which is uppercase for HTML elements in an HTML document, so the check never matched and everyautoplaymutation was recorded. A looping background video emits one on each cycle.Why: noticed while re-verifying the upstream tracker for rrweb
2.1.2— this is upstream rrweb#1921 (fixing rrweb#1916), confirmed present in our fork, so we pay for the noise today.Changes
attributesbranch (tagNameLower) and use it forignoreAttribute,transformAttribute, and theiframe/input/dialogchecks in the same block. The latter three were already correct for HTML documents; using one normalised value keeps the block consistent and makes them correct in XML documents, which preserve tag-name case.ignoreAttribute, after the tag check short-circuits — so it costs nothing for the non-media elements that make up virtually every attribute mutation.packages/rrweb/rrweb/test/record/mutation-attributes.test.ts, a jsdom probe overMutationBufferin the style of the existingmutation-blocking.test.ts. Verified it fails without the fix (expected [ 'autoplay' ] to deeply equal []) — a unit test ofignoreAttributealone would not have caught this, since the bug is entirely at the call site.Tracked in #3765. This is the same shape as upstream's fix, so it should not add fork divergence.
Verification
@posthog/rrweb+@posthog/rrweb-snapshot:tsc -noEmitandoxlint srcclean;record.test.ts(48),mutation-blocking.test.ts(14),snapshot.test.ts+utils.test.ts(141), and the new file (4) all pass.Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Found while updating the two rrweb gap trackers (#3765 / #3766) for the upstream
2.1.2release. The release itself needed nothing pulled in, but triaging the 16 upstream PRs opened since the last scan surfaced this one as a live bug in our fork, verified by reading both call sites rather than trusting the upstream description.Two judgement calls worth flagging for review:
tagName === 'video' || tagName === 'audio'rather than upstream's['video','audio'].includes(tagName)— this runs per attribute on the serialize hot path and the existing form avoids the array allocation.iframe/input/dialogcomparisons in the same block, which upstream also does for the first two. This is behaviour-neutral for HTML documents. Say the word if you would rather see the one-lineignoreAttributefix in isolation.Tools: Claude Code (Bash, Read, Edit, Write,
gh). No manual browser testing beyond the automated suites listed above.Created with PostHog Desktop