Repository navigation
Conversation
Member
Author
|
@codex review |
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. |
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 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
PostHog Review · Standard · reviewed c0a5249: 2 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
| adds: [], | ||
| }); | ||
| }); | ||
| return on('load', handler, doc); |
Contributor
There was a problem hiding this comment.
P2 · Observe image loads inside recorded shadow roots
For images inside recorded shadow roots, native load events are not composed and never reach this document listener. ShadowDomManager does not install an image observer, so images that load after serialization never receive natural-size attributes, including after src changes. Install the load observer on each tracked shadow root too.
Member
Author
There was a problem hiding this comment.
Fixed in 699d6ed. initImageSizeObserver is now exported, and ShadowDomManager.addShadowRoot installs it on each tracked shadow root (listening for load and error) and removes it with that root's other handlers. A new record test covers an image in a shadow root that loads after serialization.
Comment on lines
+1379
to
+1380
| if ((reported.get(image) ?? serialized) === size) return; | ||
| reported.set(image, size); |
Contributor
There was a problem hiding this comment.
P2 · Reset size deduplication when an image is serialized again
When a previously reported image loads a different source while detached and is then reinserted, serializeNodeWithId records its new size but reported retains the old size. Switching back to the old size returns here without emitting, leaving the recording with incorrect dimensions. Scope deduplication to the current serialized metadata or synchronize it during serialization.
Member
Author
There was a problem hiding this comment.
Fixed in 699d6ed. The dedup cache now stores the serialized node (mirror.getMeta) next to the reported size and is used only while that node is still current. Once an image is serialized again, the comparison starts from the size the new serialization recorded, so switching back to an earlier size is emitted.
Contributor
Contributor
Coherence4 spec problems · 0 failing chokepoints · 2 spec gaps · 0 guard failures 44 components, 7 bullets (5 invariants, 2 requirements), 4 problems Spec problems (spec --check)
Chokepoints: none declared. Spec gaps: 2 entrances with outside or unknown trust and no traced control, and 10 more held by the adoption baseline
Close one with |
Contributor
|
[Medium impact] Records image dimensions during session replay. The PR appears safe to merge, with non-blocking gaps in image-size capture. Reviews (1) · Last reviewed commit: "feat(replay): record the natural size of..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| adds: [], | ||
| }); | ||
| }); | ||
| return on('load', handler, doc); |
Contributor
There was a problem hiding this comment.
Image load events inside shadow roots do not reach this document listener. Images that load after serialization inside web components never get their natural size recorded, leaving consumers without dimensions when the picture is unavailable during replay. Install the observer on each recorded shadow root and remove it with that root’s other listeners.
Knowledge Base Used: Session replay recording and playback
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/rrweb/rrweb/src/record/observer.ts
Line: 1396
Comment:
**Shadow images miss their size**
Image `load` events inside shadow roots do not reach this document listener. Images that load after serialization inside web components never get their natural size recorded, leaving consumers without dimensions when the picture is unavailable during replay. Install the observer on each recorded shadow root and remove it with that root’s other listeners.
**Knowledge Base Used:** [Session replay recording and playback](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/posthog-js/-/docs/session-replay.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Member
Author
There was a problem hiding this comment.
Fixed in 699d6ed. initImageSizeObserver is now exported, and ShadowDomManager.addShadowRoot installs it on each tracked shadow root (listening for load and error) and removes it with that root's other handlers. A new record test covers an image in a shadow root that loads after serialization.
Comment on lines
+1378
to
+1379
| const serialized = `${meta.attributes.rr_naturalWidth}x${meta.attributes.rr_naturalHeight}`; | ||
| if ((reported.get(image) ?? serialized) === size) return; |
Contributor
There was a problem hiding this comment.
The reported cache survives when an image is serialized again and overrides the newer size in meta.attributes. For example, report size A, detach the image, load size B while detached, and reinsert it. The added node records B, but a later load of A is skipped because reported still contains A. Consumers then retain the wrong dimensions. Track the serialized metadata alongside the cached size, or reset the cache when that metadata changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/rrweb/rrweb/src/record/observer.ts
Line: 1378-1379
Comment:
**Reused images keep stale sizes**
The `reported` cache survives when an image is serialized again and overrides the newer size in `meta.attributes`. For example, report size A, detach the image, load size B while detached, and reinsert it. The added node records B, but a later load of A is skipped because `reported` still contains A. Consumers then retain the wrong dimensions. Track the serialized metadata alongside the cached size, or reset the cache when that metadata changes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Member
Author
There was a problem hiding this comment.
Fixed in 699d6ed. The dedup cache now stores the serialized node (mirror.getMeta) next to the reported size and is used only while that node is still current. Once an image is serialized again, the comparison starts from the size the new serialization recorded, so switching back to an earlier size is emitted.
| const serialized = `${meta.attributes.rr_naturalWidth}x${meta.attributes.rr_naturalHeight}`; | ||
| if ((reported.get(image) ?? serialized) === size) return; | ||
| reported.set(image, size); | ||
| mutationCb({ |
Contributor
There was a problem hiding this comment.
Calling mutationCb directly bypasses freezePage(). If an existing image changes src and loads while mutations are frozen, its new size is emitted immediately while the src change stays buffered. Consumers receive the new dimensions against the old source. Queue these attributes through the mutation buffer so they stay ordered with the image’s other changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/rrweb/rrweb/src/record/observer.ts
Line: 1381
Comment:
**Image changes arrive out of order**
Calling `mutationCb` directly bypasses `freezePage()`. If an existing image changes `src` and loads while mutations are frozen, its new size is emitted immediately while the `src` change stays buffered. Consumers receive the new dimensions against the old source. Queue these attributes through the mutation buffer so they stay ordered with the image’s other changes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Member
Author
There was a problem hiding this comment.
Not changing this. freezePage is only reached through the public record.freezePage() API, and the first user-initiated event unfreezes the buffers. Other recorder sources that bypass the mutation buffer (media interactions, canvas mutations, fonts) already emit during a freeze the same way. Within a freeze window the replay can briefly show the new size against the old src, but once the buffered src mutation flushes the final state is the same, and the size describes the image that actually loaded. Routing this through MutationBuffer would mean injecting synthetic attribute records into a queue built around observed DOM mutations, which costs more than this short window is worth.
Contributor
|
Size Change: +36.8 kB (+0.15%) Total Size: 24.2 MB 📦 View Changed
ℹ️ View Unchanged
|
![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: c0a5249074
ℹ️ 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".
Comment on lines
+1386
to
+1389
| attributes: { | ||
| rr_naturalWidth: naturalWidth, | ||
| rr_naturalHeight: naturalHeight, | ||
| }, |
There was a problem hiding this comment.
When a previously loaded image changes src/srcset, these generated attributes remain associated with the node until another successful load. If the replacement fails, is still pending when recording ends, or is removed downstream, consumers will pair the new URL with the old image's dimensions and recreate the same incorrect layout this feature is intended to prevent. Emit a removal/reset for both values when the selected source is invalidated, before publishing a later successful size.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Fixed in 699d6ed for the failed-load case. The observer now also listens for error, and when an image ends up with no natural size it emits rr_naturalWidth: null / rr_naturalHeight: null, which removes the attributes on replay. The record test now switches to a broken source and asserts the clear. Known limitation: a replacement still in flight when recording stops keeps the previous size, because nothing fires to clear it. Clearing it on the src mutation itself would race the load event under a frozen mutation buffer and could wipe a correct size.
| adds: [], | ||
| }); | ||
| }); | ||
| return on('load', handler, doc); |
There was a problem hiding this comment.
For an image serialized while still loading inside a shadow root, the load event is non-composed and therefore never reaches this document-level listener. Although rrweb explicitly records shadow-root mutations through ShadowDomManager, no corresponding image-size listener is installed there, so late-loading shadow images never receive rr_naturalWidth/rr_naturalHeight. Register this observer for each tracked shadow root as well.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Fixed in 699d6ed. initImageSizeObserver is now exported, and ShadowDomManager.addShadowRoot installs it on each tracked shadow root (listening for load and error) and removes it with that root's other handlers. A new record test covers an image in a shadow root that loads after serialization.
| attributes: { | ||
| [key: string]: string | styleOMValue | null; | ||
| // number: generated properties such as rr_naturalWidth | ||
| [key: string]: string | number | styleOMValue | null; |
There was a problem hiding this comment.
attributeMutation is exported from the published @posthog/rrweb-types entry point, and widening this received-value union can break TypeScript consumers that previously handled only strings, style objects, and null. The maintainer check could not run, so the repository instructions require treating the author as external; no API-defining sdk-spec or agreed issue for this exported change is identified, so this public API change needs to be explicitly noted before landing.
AGENTS.md reference: AGENTS.md:L5-L9
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Addressed in 699d6ed. The changeset now bumps @posthog/rrweb, @posthog/rrweb-snapshot and @posthog/rrweb-types alongside posthog-js, and states that the value type of attributeMutation.attributes widens to include number (for generated attributes such as rr_naturalWidth), so consumers that narrow those values exhaustively need a number branch.
Contributor
|
This branch has not been 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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
A replay that cannot load a picture loses the space the page gave it.
The image URL may have expired, sit behind a login, or have been dropped by an anonymizer downstream.
A page often leaves an
<img>unsized and lets the picture's own size set the box.Without the picture, the replayed box either collapses or, when something substitutes a size-less placeholder, stretches to the width of its container and covers the content around it.
Nothing after the browser can recover that size: the recording carries only the URL, and once the bytes are gone the size is gone with them.
PostHog's ML labeling replays hit this daily. In one reported case a 20 px status icon in a quiz came back as a 436 px square and hid the answer below it.
Changes
<img>that has already loaded is serialized with two numeric generated attributes,rr_naturalWidthandrr_naturalHeight.loadlistener on the document (initImageSizeObserver, underrecordDOM) reports the size of an image that loads after it was serialized, or that loads a newsrc, as an attribute mutation carrying only those two numbers. It skips blocked images, unloaded or broken images (naturalWidth0), and a size already recorded for that element.rebuildalready ignore unknown numericrr_*attributes. The replayer's style branch now checks that the value is an object, since the mutation type widens to allow numbers.attributeMutation.attributesvalues widen tostring | number | styleOMValue | null, likeattributesalready does forrr_scrollLeftand media state.naturalWidthis the decoded image's size, not a measurement of the page.A consumer can then size a stand-in for a picture it could not load, while the page's own CSS keeps priority. Using the size in PostHog's own player is left for a follow-up.
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset fileHow did you test this code?
srcchange reports the new size once. It passed 5 of 5 runs.robot.pngintegration expectation and three blob-url recorder snapshots gained exactly the new size (the snapshot diff adds only those mutations).check-typesacross the dependents includingposthog-js, and oxlint pass locally.🤖 Agent context
Autonomy: Human-driven (agent-assisted)