Add support for uploading crash reports to Sentry #2720
Conversation
…ialog look good on iPad
github-actions
bot
added
the
bot: not in app board
ayoy
removed
the
bot: not in app board
| ZStack { | ||
| Rectangle() | ||
| .foregroundColor(Color(designSystemColor: .container)) | ||
| .frame(maxWidth: .infinity, maxHeight: .infinity) | ||
| .cornerRadius(4.0) | ||
|
|
||
| Text(reportDetails) | ||
| .multilineTextAlignment(.leading) | ||
| .font(.crashReport) | ||
| .foregroundColor(Color(designSystemColor: .textSecondary)) | ||
| .padding(24) | ||
| } |
There was a problem hiding this comment.
I was wondering whether it would make sense to put this in a scroll view which is horizontally scrollable since text wrapping can make it awkward to read, but TBH there's no right approach here – happy to leave this as-is.
There was a problem hiding this comment.
That was my first idea and implementation, but we agreed with Bryan that it actually shouldn't be scrolled horizontally (see the discussion).
That said, the scroll view approach makes rendering faster (not fast, but faster) 🤷
| VStack(spacing: 0) { | ||
| ScrollView { | ||
| VStack(spacing: 24) { | ||
| Image("Breakage-128") |
There was a problem hiding this comment.
We could use Image(decorative:) here to make sure that VoiceOver API doesn't read the image name (it does this in the current implementation).
There was a problem hiding this comment.
Of course :) thanks for pointing that out :) I'll fix it right away.
| CrashCollection.start { | ||
| Pixel.fire(pixel: .dbCrashDetected, withAdditionalParameters: $0, includedParameters: []) | ||
| crashCollection.start { pixelParameters, payloads, sendReport in | ||
| pixelParameters.forEach { params in |
There was a problem hiding this comment.
Previously the callback was called for every crash report found. Now we have a single callback that can contain multiple crash reports. We still send pixels for all crash reports, and we present onboarding for Sentry as needed using the most recent crash report (this is the same behavior as on macOS for both DMG and App Store builds).
| ZStack { | ||
| Rectangle() | ||
| .foregroundColor(Color(designSystemColor: .container)) | ||
| .frame(maxWidth: .infinity, maxHeight: .infinity) | ||
| .cornerRadius(4.0) | ||
|
|
||
| Text(reportDetails) | ||
| .multilineTextAlignment(.leading) | ||
| .font(.crashReport) | ||
| .foregroundColor(Color(designSystemColor: .textSecondary)) | ||
| .padding(24) | ||
| } |
There was a problem hiding this comment.
That was my first idea and implementation, but we agreed with Bryan that it actually shouldn't be scrolled horizontally (see the discussion).
That said, the scroll view approach makes rendering faster (not fast, but faster) 🤷
|
Thanks for the review @samsymons! Yes, the crash report is very slow to load (also relatively slow on a modern device), but there's not much we can do about it. The slowness comes from the UI rendering, not from processing crash report payload – so it can't be offloaded to a background thread. Showing a spinner wouldn't really work either (or the spinner wouldn't spin). The working solution would be to break the text into lines and load in a list with reusable cells but that's too much :) But as you said, it's only presented once to the user and also it's not a core flow. |
# By Daniel Bernal (11) and others # Via Mariusz Śpiewak (3) and others * main: (63 commits) Release 7.115.1-1 (#2770) Fix iOS auto-clearing fails to remove cookies (#2769) Release 7.115.1-0 (#2768) Fix iOS auto-clearing fails to remove cookies (#2767) iOS: VPN screen improvements (#2721) Release 7.116.0-2 (#2763) Remove "Thank You" prompt (#2762) Test to ensure that page refresh doesn't affect url bar focus (#2749) Add support for uploading crash reports to Sentry (#2720) Replace SwiftLintPlugin with SwiftLintTool (#2710) Release 7.116.0-1 (#2761) Remove validator app (#2754) Fix crash when quickly adding/removing tabs in switcher (#2760) Fix settings navigation bar colors after reopening (#2758) Alpha ad-hoc lane (#2492) Keep a weak reference to UserScriptMessageBroker (#2755) Add refresh config cell to top of debug (#2735) VPN: Replace available interfaces in VPN metadata (#2750) Require device auth to be set in order to use Sync (#2722) Add new iOS pixels for measuring navigation (#2730) ... # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo/AppDelegate.swift # DuckDuckGo/MainViewController.swift
Task/Issue URL: https://app.asana.com/0/72649045549333/1205862129634403/f
Description:
This change introduces uploading crash reports to DDG's Sentry instance. Users are prompted
to opt in to crash collection upon launching the app after it crashed. A user can decide either
to always send reports or never send them. A switch is added to Settings to control their preference.
Steps to test this PR:
Copy Testing:
’rather than'Orientation Testing:
Device Testing:
OS Testing:
Theme Testing:
Internal references:
Software Engineering Expectations
Technical Design Template