Repository navigation
PANA-8613: View traversal and decomposition - #3728
jonathanmos wants to merge 1 commit into
Conversation
|
✅ All CI checks and tests passed. Datadog automation helped this PR pass. 🎉 All green!🧪 All tests passed 🔄 Datadog retried 2 tests - 2 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: ff3645c | Docs | View more details | Give us feedback! |
1dd5cfc to
50400d0
Compare
92ae883 to
5ce8684
Compare
50400d0 to
35e15c5
Compare
6e5525f to
e67102f
Compare
06dc1f8 to
783469c
Compare
0168738 to
a4083d3
Compare
783469c to
902b1be
Compare
a4083d3 to
5287820
Compare
a21f246 to
357bf07
Compare
294da51 to
fd9fa08
Compare
5287820 to
5b481bc
Compare
bd69260 to
751e07a
Compare
883ec03 to
b94ea39
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)
ℹ️ 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. |
![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: b94ea39e26
ℹ️ 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".
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: b94ea39e26
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
561fb40 to
96247a0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96247a0233
ℹ️ 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".
96247a0 to
a27f444
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a27f444b06
ℹ️ 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".
| val inheritedClip = if (item.clipToBounds) item.ancestorClip.intersectWith(bounds) else item.ancestorClip | ||
| val clip = view.clipBoundsOnScreen(state.screenDensity)?.let { inheritedClip.intersectWith(it) } | ||
| ?: inheritedClip |
There was a problem hiding this comment.
When a view uses clipToOutline = true, such as a rounded card or an oval container, Android clips the view and its descendants to the outline, but this path only incorporates rectangular ancestor and clipBounds constraints. The replay therefore renders child content and backgrounds into corners or regions that are absent on screen. Resolve the view outline into a layer clip modifier, or otherwise fall back when the outline cannot be represented.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
will be addressed in a followup pr
| val orderedWindows = orderedDecorViews.mapNotNull { root -> | ||
| trackedByDecorView[root] ?: windowFromDecorView(root) | ||
| } |
There was a problem hiding this comment.
When an untracked root cannot be converted to a Window—notably a PopupWindow root, or a dialog decor when private-field reflection is blocked—it remains in decorViews and is captured visually, but mapNotNull silently omits it from windows. No CompositionWindowTouchCallback is then installed for that root, so every interaction inside the displayed popup or dialog is absent from the replay. Provide a root-level touch fallback for unresolved roots rather than dropping them only from touch interception.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
will be addressed in a followup pr
jonathanmos
added
the
ready-for-human-review
a27f444 to
b4e4f19
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4e4f19040
ℹ️ 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".
|
|
||
| collectTouchOverrideArea(view, state) | ||
|
|
||
| val bounds = viewBoundsResolver.resolveViewGlobalBounds(view, state.screenDensity).toCaptured() |
There was a problem hiding this comment.
For any view with non-default scaleX, scaleY, or rotation—such as during a common property animation—the bounds resolver still returns the untransformed width and height, and the resulting layer records only an opacity modifier. Its text, background, and subtree are therefore replayed as an unscaled, unrotated rectangle even though Android renders a transformed view. Encode the spatial transform on the layer or capture the transformed output instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
will be handled in a later pr
18b7912 to
047ce90
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 047ce90f99
ℹ️ 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".
| override fun map(view: View, mappingContext: CapturedMappingContext): CapturedViewMapperResult { | ||
| val style = backgroundShapeStyleResolver.resolve(view, internalLogger) | ||
| ?: return CapturedViewMapperResult.None |
There was a problem hiding this comment.
For a ViewGroup with a foreground drawable—such as a FrameLayout using a translucent scrim, selector, or ripple over its contents—the fallback mapper resolves only view.background, so the foreground is absent from the replay. This cannot be fixed by simply adding it to the returned wireframes because traversal appends all mapper output before child layers, while Android draws the foreground after the children. Emit a distinct foreground wireframe from the layer-finishing step so it retains its frontmost paint order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
will be handled in a later pr
047ce90 to
4a37ef8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a37ef8384
ℹ️ 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".
| override fun map(view: View, mappingContext: CapturedMappingContext): CapturedViewMapperResult { | ||
| val style = backgroundShapeStyleResolver.resolve(view, internalLogger) | ||
| ?: return CapturedViewMapperResult.None |
There was a problem hiding this comment.
When composition-tree recording is enabled, the registry has dedicated mappers only for WebView and TextView, so a normal ImageView reaches this fallback. The fallback examines only view.background; an ImageView commonly has no background because its visible icon or photo is stored in ImageView.drawable, causing it to return None and emit an empty layer. As a result, standard images and image buttons disappear from replay; add an ImageView mapper or pixel fallback that captures the displayed drawable subject to image privacy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
will be handled in a later pr
4a37ef8 to
dbee73a
Compare
There was a problem hiding this comment.
💡 Codex Review
Re-wrap callbacks replaced on still-active windows
When another library replaces a tracked window's Window.Callback without delegating to the previously installed callback, subsequent periodic and lifecycle refreshes continue to classify the window as already wrapped solely because it remains a key in wrappedWindows. Touch recording then stays disabled for that window indefinitely, even though it remains active. During intercept, also verify that the installed callback is still the stored wrapper and re-wrap the current callback when it is not.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a6b4cd5d7
ℹ️ 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".
5a6b4cd to
09c4dad
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09c4dadc30
ℹ️ 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".
09c4dad to
0539057
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05390571af
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42859873bf
ℹ️ 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".
| wireframes += CapturedWireframe.Text( | ||
| identity = mappingContext.identityFactory.textWireframe(mappingContext.ownerIdentity), | ||
| bounds = capturedBounds, | ||
| text = resolveLayoutText(view), |
There was a problem hiding this comment.
When an empty EditText or TextView displays a hint, Android renders that hint through a separate hint layout while layout.text/text remains empty. Serializing only resolveLayoutText(view) therefore emits an empty text wireframe, so placeholders such as form-field labels disappear from the replay even though the hint color is captured. Select the visible hint when the text is empty.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
will be handled in a later pr
4285987 to
1de9370
Compare
💡 Codex Review Map touch overrides through the view transform
When a ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
Enabling composition recording allows unmasked text to reach the upload path despite the configured privacy policy. Occlusion also drops visible content during legacy View animations because their drawing transformations are not considered.
🤖 Bits Code Review · Commit 1de9370
| windowSource, | ||
| rumContextProvider | ||
| -> | ||
| AndroidCapturedSnapshotProducer( |
There was a problem hiding this comment.
Enabling composition recording with the default TextAndInputPrivacy.MASK_ALL now captures unmasked TextView and EditText contents. ImmediateCapturedSnapshotProcessor forwards these snapshots unchanged, and DefaultSnapshotCompletionProcessor writes their text to RecordWriter. Although masking is planned for a later workstream, publication is already enabled here, allowing sensitive text to be uploaded despite the configured privacy policy. Apply privacy before publication or keep these snapshots out of the write path until masking exists.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
will be handled in a later pr
| for (i in childCount - 1 downTo 0) { | ||
| val child = children[i] | ||
| // A transformed child cannot be represented by its layout rectangle in either direction. | ||
| if (hasScaleOrRotation(child)) continue |
There was a problem hiding this comment.
A legacy TranslateAnimation or AlphaAnimation changes drawing without updating the View properties checked by this detector. An opaque sibling can therefore be treated as covering its original rectangle while actually moved or translucent, causing visible content beneath it to be omitted from replay. Skip animated children in both directions of the occlusion comparison unless their effective transformation and opacity are known.
| if (hasScaleOrRotation(child)) continue | |
| if (child.animation != null || hasScaleOrRotation(child)) continue |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
will be handled in a later pr
a6081e5 to
de0128e
Compare
1de9370 to
978c258
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 978c258201
ℹ️ 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".
| traversal = AndroidWindowTraversal( | ||
| mapperRegistry = builtInCapturedMappers(sdkCore.internalLogger, customDrawableMappers), | ||
| internalLogger = sdkCore.internalLogger | ||
| ), |
There was a problem hiding this comment.
When this pipeline is enabled for a Compose screen, the default factory constructs AndroidWindowTraversal without a composeHostCallback, leaving its default value null. ComposeView is therefore never handed off for semantic decomposition and instead reaches the background-only fallback; because its rendered Compose content is not represented by ordinary native child wireframes, Compose-only screens produce effectively blank replays. Supply the Compose capture callback here or retain a pixel fallback for unmapped Compose hosts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
handled in a later branch
de0128e to
0f3bad7
Compare
978c258 to
ff3645c
Compare
| * WebView slot). Traversal must not also capture that view's native children independently. | ||
| */ | ||
| data class Wireframes( | ||
| val wireframes: List<CapturedWireframe>, |
There was a problem hiding this comment.
nit: don't you though of declaring this parameter as vararg CapturedW ireframe? Then for the single only item you wouldn't need to create list of one element?
Or maybe add a secondary constructor to make call site more simple?
Also could be applied to other classes like that
| private val drawableToColorMapper: DrawableToColorMapper = DrawableToColorMapper.getDefault() | ||
| ) { | ||
| fun resolve(view: View, internalLogger: InternalLogger): CapturedShapeStyle? { | ||
| val color = view.background?.let { drawableToColorMapper.mapDrawableToColor(it, internalLogger) } |
There was a problem hiding this comment.
nit: internalLogger is only forwarded to drawableToColorMapper.mapDrawableToColor(...), and it is already available at wiring time in DefaultCompositionPipelineFactory.builtInCapturedMappers — could we inject it in the constructor instead of threading it through every resolve(...) call? resolve then loses the parameter, and CapturedTextViewMapper / CapturedViewGroupFallbackMapper can drop their own internalLogger as well, since they only pass it along.
internal class CapturedBackgroundShapeStyleResolver(
private val internalLogger: InternalLogger,
private val colorStringFormatter: ColorStringFormatter = DefaultColorStringFormatter,
private val drawableToColorMapper: DrawableToColorMapper = DrawableToColorMapper.getDefault()
) {
fun resolve(view: View): CapturedShapeStyle? =
view.background
?.let { drawableToColorMapper.mapDrawableToColor(it, internalLogger) }
?.let { color ->
// View alpha belongs to the owning layer so text and descendants share it exactly once.
CapturedShapeStyle(
backgroundColor = colorStringFormatter.formatColorAsHexString(color)
)
}
}Factory wiring:
val backgroundResolver = CapturedBackgroundShapeStyleResolver(
internalLogger = internalLogger,
drawableToColorMapper = DrawableToColorMapper.getDefault(customDrawableMappers)
)and the call sites become just backgroundShapeStyleResolver.resolve(view).
| Typeface.SANS_SERIF -> SANS_SERIF_FAMILY_NAME | ||
| Typeface.MONOSPACE -> MONOSPACE_FAMILY_NAME | ||
| Typeface.SERIF -> SERIF_FAMILY_NAME | ||
| else -> SANS_SERIF_FAMILY_NAME |
There was a problem hiding this comment.
should we log such case into telemetry for the any update in the future ?
| } | ||
|
|
||
| private fun resolvePadding(textView: TextView, pixelsDensity: Float): CapturedPadding { | ||
| return if (textView.layout != null) { |
There was a problem hiding this comment.
please add a comment that explains why we 're checking for the layout here
| messageBuilder = { "No mapper found for view $viewType" }, | ||
| throwable = null, | ||
| onlyOnce = true, | ||
| additionalProperties = mapOf("replay.widget.type" to viewType) |
There was a problem hiding this comment.
Just in case: I see that log is going to be written only once, but map construction logic would be executed for every call anyway. In case if this code would be executed frequently - it might add an additional pressure to the GC. Consider check such case and maybe move attributes to the message?
| } | ||
| is WorkItem.VisitTouchPrivacy -> visitTouchPrivacy(item, stack, state) | ||
| is WorkItem.Finish -> { | ||
| completeLayer( |
There was a problem hiding this comment.
maybe it's easier to just pass item as a parameter to the method? or make completeLayer an extension for item ?
|
|
||
| @UiThread | ||
| private fun tryVisit(view: View, expectedParent: ViewParent?, state: TraversalState): Boolean = | ||
| !viewUtilsInternal.isNotVisible(view) && |
There was a problem hiding this comment.
double negative statement. maybe could be replaced with something like viewUtilsInternal.isVisible(view)?
| } | ||
|
|
||
| private sealed interface ResultSink { | ||
| data class ChildOf(val list: MutableList<CapturedChild>) : ResultSink |
There was a problem hiding this comment.
having mutable list inside data class is an anti-pattern
| private fun <T> ArrayDeque<T>.pop(): T = removeLast() | ||
|
|
||
| @Suppress("UnsafeThirdPartyFunctionCall") // pure size check, cannot throw | ||
| private fun <T> ArrayDeque<T>.isNotStackEmpty(): Boolean = isNotEmpty() |
There was a problem hiding this comment.
why isNotEmpty is not enough here?
| CapturedMapperTypeWrapper(WebView::class.java, CapturedWebViewMapper()), | ||
| CapturedMapperTypeWrapper( | ||
| TextView::class.java, | ||
| CapturedTextViewMapper( |
There was a problem hiding this comment.
Not sure that we need dedicated CapturedMapperTypeWrapper here. Maybe it's worth to make supportsView a method of *Mapper?
3 participants
What does this PR do?
Adds native View traversal and snapshot production to the composition-based Session Replay pipeline.
Motivation
Connect the existing capture orchestration to the Android View hierarchy so the composition pipeline can produce native snapshots.
Review checklist (to be filled by reviewers)