PANA-5681: Integrate HeatmapIdentifierRegistry into Session Replay for heatmaps [4 of 4]#3206
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39a5a2b5af
ℹ️ 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".
39a5a2b to
2f8955a
Compare
This comment has been minimized.
This comment has been minimized.
ViewIdentityResolver into Session Replay for heatmapsViewIdentityResolver into Session Replay for heatmaps [4 of 4]
cbfbf76 to
9a2e330
Compare
ViewIdentityResolver into Session Replay for heatmaps [4 of 4]HeatmapIdentifierRegistry into Session Replay for heatmaps [4 of 4]
HeatmapIdentifierRegistry into Session Replay for heatmaps [4 of 4]9a2e330 to
e63b7f4
Compare
6210b4b to
fa8baf8
Compare
803a051 to
11ba9b6
Compare
0014da6 to
03802d0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03802d0335
ℹ️ 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".
03802d0 to
acb5749
Compare
0xnm
left a comment
There was a problem hiding this comment.
I briefly went through the production files and left some comments, I didn't cover tests.
Given the size of this PR we need to have more several reviewers here.
| import com.datadog.android.internal.heatmaps.HeatmapIdentifierRegistryProvider | ||
|
|
||
| /** | ||
| * Session Replay may be initialized before RUM — the SDK does not mandate a registration order. |
There was a problem hiding this comment.
Do we actually need to support this case? Cannot we print a warning that RUM is not yet initialized during Session Replay start?
By looking at https://docs.datadoghq.com/session_replay/mobile/setup_and_configuration/?platform=android it is not clear from code snippets that RUM should be initialized first, though (it is only mentioned that RUM SDK setup should be completed).
There was a problem hiding this comment.
The problem is that without this mechanism, the heatmaps registry is resolved once at SessionReplay.enable() time. If RUM hasn't registered yet then the registry is permanently null and heatmaps are silently disabled for the entire session. Printing a warning means heatmaps are still broken, and the user has no way to recover without restarting the app.
There was a problem hiding this comment.
Would it make sense to add permanentId to the _common-wireframe-schema.json file? So we don't need to do these mappings.
There was a problem hiding this comment.
It's possible to put permanentId on the common schema but the reason we didn't do this is that it breaks the previous ordering of arguments for the wireframe constructors instead of permanentId being the last argument, and it also wouldn't remove the need for these mappings. The root issue is that .copy() only exists on data classes, not on the Wireframe sealed class, so to stamp permanentId onto a wireframe subtype we have to downcast via when. To avoid that we'd need to touch the schema generator to emit a withPermanentId() method on each subtype that delegates to its own copy().
698a9eb to
eeca598
Compare
hamorillo
left a comment
There was a problem hiding this comment.
I went through the production code and haven't found any blockers. That said, the PR is quite big (so it's not easy to review), and it's my first time reviewing SR code, so I'm not confident at all, as I spend most of the time trying to understand everything.
I'll try to come back later with fresh eyes.
What does this PR do?
Wires
ViewIdentityResolverinto Session Replay to populatepermanentIdon wireframes. This enables correlating wireframes with RUM action events for heatmap visualization.Key changes:
ViewIdentityProviderinterface for Session Replay's view identity resolutionViewIdentityResolver(from RUM feature context) viaViewIdentityResolverAdapterviewIdentityProvidertoMappingContextfor use by all wireframe mapperspermanentIdin all wireframe types (Shape, Text, Image, Placeholder, Webview)onWindowRefreshed()on each draw pass to index the current view hierarchyMotivation
Support mobile heatmaps. This is the final piece connecting Session Replay wireframes to RUM actions.
Additional Notes
MappingContextForgeryFactoryinsession-replay-composeandsession-replay-materialtest suites.Review checklist (to be filled by reviewers)