Add SR support for embedded content wireframes - #3701
Conversation
PANA-8391: Add embedded content wireframe model support
…evelop-into-flutter-support
…lutter-support Merge develop into feature/flutter-support
…lutter-support2 Merge develop into feature/flutter-support
PANA-8412: Support cross-platform embedded content in Session Replay
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 10615a8 | Docs | Datadog PR Page | Give us feedback! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10615a8e70
ℹ️ 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".
| "slotId": { | ||
| "type": "string", | ||
| "description": "Unique ID of the slot that generated this record.", | ||
| "readOnly": true |
There was a problem hiding this comment.
Preserve the existing record constructors
Adding this optional generated property changes MobileFullSnapshotRecord from the JVM constructor (Long, Data) to (Long, Data, String?); the analogous change in incremental-snapshot-record-schema.json also removes the existing two-argument MobileIncrementalSnapshotRecord constructor. Kotlin default arguments do not retain those old JVM signatures, so an already-compiled library that constructs either public model will fail with NoSuchMethodError when resolved against this SDK version. Keep binary-compatible two-argument overloads or secondary constructors while adding slotId.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We don’t guarantee binary compatibility for the generated Session Replay models, including record and wireframe types. Therefore, preserving the previous JVM constructors is not required for this change.
| is MobileSegment.Wireframe.TextWireframe -> this.hasOpaqueBackground() | ||
| is MobileSegment.Wireframe.PlaceholderWireframe -> true | ||
| is MobileSegment.Wireframe.WebviewWireframe -> true | ||
| is MobileSegment.Wireframe.EmbeddedContentWireframe -> this.isVisible != false |
There was a problem hiding this comment.
Respect transparent embedded host views
When an embedded host has partial or zero alpha, or a translucent background, the mapper carries that opacity in its shape style but this branch still treats the wireframe as fully opaque solely because it is visible. NodeFlattener consequently removes any fully covered native wireframes underneath, so transparent portions of the embedded renderer cannot be reconstructed in the replay. Determine coverage from the host opacity/shape style, or conservatively treat the embedded wireframe as non-opaque when opacity is unknown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Transparent Flutter hosts appear to be a valid use case, but I don’t want to block this PR. I’ll open a follow-up to address this separately.
| ) : ResourceProcessor { | ||
|
|
||
| override fun process(identifier: String, data: ByteArray, mimeType: String?) { | ||
| if (!resourceDataStoreManager.markResourceAsSentIfNew(identifier)) { |
There was a problem hiding this comment.
Mark embedded resources only after persistence succeeds
For the new embedded-resource path, this marks and persists the identifier as sent before SessionReplayResourcesWriter attempts to store the bytes. That writer ignores the Boolean result of EventBatchWriter.write and can also no-op if its feature disappears, so if the first write is rejected during a transient storage failure or shutdown race, every later submission with the same identifier is suppressed and replay wireframes permanently reference a missing resource. Mark the identifier only after a successful storage write, or remove it again when persistence fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This matches the existing native Session Replay resource flow, which also marks resources before writing them. Fixing it would require changing the async writer contract, so I’d handle this in a separate pr
What does this PR do?
Merges feature/flutter-support into develop, adding support for embedded content wireframes. This is required to support Flutter hybrid views in replays. This PR won't actually be merged until there is a final confirmation from the flutter side that this meets all requirements.
Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)