-
-
Notifications
You must be signed in to change notification settings - Fork 453
Fix broken view hierarchy retrieval for Jetpack Compose 1.8+ #4485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix broken view hierarchy retrieval for Jetpack Compose 1.8+ #4485
Conversation
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
f973d89 | 478.56 ms | 510.76 ms | 32.20 ms |
e406a73 | 405.00 ms | 420.52 ms | 15.52 ms |
72db3dd | 466.26 ms | 550.76 ms | 84.50 ms |
6847d1a | 404.77 ms | 414.66 ms | 9.89 ms |
3e78fe2 | 410.08 ms | 462.62 ms | 52.54 ms |
be72013 | 432.08 ms | 494.88 ms | 62.80 ms |
6e02352 | 451.73 ms | 477.94 ms | 26.20 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
f973d89 | 1.58 MiB | 2.08 MiB | 511.62 KiB |
e406a73 | 1.58 MiB | 2.08 MiB | 511.61 KiB |
72db3dd | 1.58 MiB | 2.08 MiB | 511.64 KiB |
6847d1a | 1.58 MiB | 2.08 MiB | 511.63 KiB |
3e78fe2 | 1.58 MiB | 2.08 MiB | 511.73 KiB |
be72013 | 1.58 MiB | 2.08 MiB | 511.73 KiB |
6e02352 | 1.58 MiB | 2.12 MiB | 549.10 KiB |
...roid-replay/src/main/java/io/sentry/android/replay/viewhierarchy/ComposeViewHierarchyNode.kt
Show resolved
Hide resolved
...roid-replay/src/main/java/io/sentry/android/replay/viewhierarchy/ComposeViewHierarchyNode.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a little doubt, but looks good!
) | ||
} | ||
|
||
// If we're unable to retrieve the semantics configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking if we should check if any masking is actually enabled? Thinking of a case where nothing should be masked, but we still would weirdly mask something when failed to retrieve the semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(maskAllText and maskAllImages, that is)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should even process the view hierarchy if no masking is active? I'll get this merged now, as by default masking is active anyway. We can always improve on top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one comment, but otherwise LGTM, great workaround and tests!
📜 Description
Fixes missing masking for Jetpack Compose 1.8+ as
LayoutNode.collapsedSemantics
was removed in favor ofLayoutNode.getSemanticsConfiguration()
We used to swallow errors when retrieving the Compose ViewHierarchy, leading to un-masked replays. After some discussion we're changing this to a more defensive approach: If parts of the View Hierarchy can't be retrieved we simply mask the relevant parts of the UI.
💡 Motivation and Context
Fixes #4467
💚 How did you test it?
Added tests + manual testing.
Before:

After:

📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps