Skip to content
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: Session replay transformed view masking #4529

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

brustolin
Copy link
Contributor

📜 Description

Fixing some masking regression and added test to track it.

💡 Motivation and Context

💚 How did you test it?

Unit tests

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Copy link

github-actions bot commented Nov 12, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1234.49 ms 1250.29 ms 15.80 ms
Size 22.30 KiB 731.06 KiB 708.76 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
879fb28 1243.18 ms 1255.98 ms 12.80 ms
98752f3 1226.18 ms 1251.38 ms 25.20 ms
e0904ef 1231.85 ms 1252.38 ms 20.53 ms
3f6c30b 1252.98 ms 1266.22 ms 13.24 ms
26530fe 1233.98 ms 1250.06 ms 16.08 ms
742d4b6 1196.56 ms 1216.54 ms 19.98 ms
8b1c6a9 1216.92 ms 1230.90 ms 13.98 ms
643853e 1225.75 ms 1247.00 ms 21.25 ms
8e76be4 1272.67 ms 1286.38 ms 13.71 ms
dacf894 1223.96 ms 1250.41 ms 26.45 ms

App size

Revision Plain With Sentry Diff
879fb28 22.84 KiB 402.88 KiB 380.03 KiB
98752f3 20.76 KiB 435.09 KiB 414.33 KiB
e0904ef 21.58 KiB 614.64 KiB 593.06 KiB
3f6c30b 22.85 KiB 408.88 KiB 386.03 KiB
26530fe 21.58 KiB 714.93 KiB 693.35 KiB
742d4b6 21.58 KiB 546.20 KiB 524.62 KiB
8b1c6a9 21.58 KiB 706.97 KiB 685.38 KiB
643853e 21.58 KiB 655.73 KiB 634.15 KiB
8e76be4 20.76 KiB 427.66 KiB 406.89 KiB
dacf894 20.76 KiB 426.34 KiB 405.58 KiB

Previous results on branch: fix/masking

Startup times

Revision Plain With Sentry Diff
65d2b7d 1223.29 ms 1238.06 ms 14.77 ms
3aa67df 1213.92 ms 1240.45 ms 26.53 ms

App size

Revision Plain With Sentry Diff
65d2b7d 22.30 KiB 730.75 KiB 708.45 KiB
3aa67df 22.30 KiB 730.74 KiB 708.44 KiB

Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.595%. Comparing base (d9f518a) to head (171c2b2).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4529       +/-   ##
=============================================
+ Coverage   91.566%   91.595%   +0.028%     
=============================================
  Files          615       615               
  Lines        69701     69988      +287     
  Branches     24967     25051       +84     
=============================================
+ Hits         63823     64106      +283     
- Misses        5787      5790        +3     
- Partials        91        92        +1     
Files with missing lines Coverage Δ
Sources/Swift/Tools/SentryViewPhotographer.swift 93.750% <100.000%> (+1.344%) ⬆️
Sources/Swift/Tools/UIRedactBuilder.swift 93.805% <100.000%> (+0.083%) ⬆️
...ests/SentryTests/SentryViewPhotographerTests.swift 99.635% <100.000%> (+0.106%) ⬆️

... and 18 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9f518a...171c2b2. Read the comment docs.

Tests/SentryTests/SentryViewPhotographerTests.swift Outdated Show resolved Hide resolved

let parentView = UIView(frame: CGRect(x: 0, y: 0, width: 50, height: 25))
parentView.addSubview(label1)
parentView.clipsToBounds = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: If we want to test clipsToBounds, shouldn't the label1 be bigger than the parentView. Currently, they are the same size, and clipsToBounds shouldn't make a difference. Or is there another reason to use it?

I think if you set the background color of the parentView to red and increase the height of the label1 to 30, no red color should be visible, cause the label and the topView should be drawn over the parentView.

Copy link
Contributor Author

@brustolin brustolin Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this test. I just need the parent to clip its bounds because it changes the flow of masking.

Tests/SentryTests/SentryViewPhotographerTests.swift Outdated Show resolved Hide resolved
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants